-
Notifications
You must be signed in to change notification settings - Fork 114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add initial type hints #128
Conversation
Unicode strings are the default behaviour in Python 3
Thanks for adding hints and cleaning up all the docstrings.
Let me know if/once you're done with this PR and I can merge and do any cleanup. |
Sorry for the delay, I must've missed the notification.
The (IDE/linter/type checker) should be able to already correctly guess the type for that:
Basically I don't understand what we're doing here, is it for Python 2 compat? Lines 23 to 26 in fc21aad
Which leads to not being sure what type to annotate query here and here:Line 933 in fc21aad
Lines 811 to 820 in fc21aad
I think it should be Union[re.Pattern, str] but I'm not sure
|
Got it. That sounds fine then. For |
I just realized In some places the args are optional but it's not mentioned in the docstrings, so I'll fix that too: Lines 866 to 871 in f0d066f
Question: is there a standardized docstring format we're using here? If not (in the docstrings only, not in the typing hints), since they're comments, we could replace Unions with with |
Yup, the repo uses Google-style docstrings (https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html). From a quick look at that page, it seems that Sphinx supports/knows about native type hints now. As such, I think we could just remove the typing info from the comments. If you want to investigate, the docs can be built via the |
Only user-facing methods have typing info for now
357168a
to
3c7cc55
Compare
|
Sphinx supports PEP 484 type annotations now
Fixed all my mistakes, Sphinx builds the docs successfully. There's a lot of warnings but none of them are about typing, almost all are Should be ready for review |
Thanks for your work! I've merged the changes into |
Closes #118
sync=True
or similar bools, it's not neededfind
andfindLabel
- I don't understand what we're doing with the regexp types ¯\_(ツ)_/¯ , I'd like to add typing to those too but I'd need some help understanding what's going onThis doesn't address the
update
method shadowing on line 621, or the unusedresync
arguments