Closed PrOF-kk closed 1 year ago
Thanks for adding hints and cleaning up all the docstrings.
bool
s implicitly typed in some way?re.Pattern
now that we don't care about Python 2 (if you're specifically referring to the docstringsresync
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.
- Are
bool
s implicitly typed in some way?
The (IDE/linter/type checker) should be able to already correctly guess the type for that:
I didn't add : bool
because the method signature is already pretty messy and didn't want to make it worse
- IIRC, we can just reference
re.Pattern
now that we don't care about Python 2 (if you're specifically referring to the docstrings
Basically I don't understand what we're doing here, is it for Python 2 compat? https://github.com/kiwiz/gkeepapi/blob/fc21aad6f33b26f4f8c78117d2fca71f675763b6/gkeepapi/__init__.py#L23-L26
Which leads to not being sure what type to annotate query
here and here:
https://github.com/kiwiz/gkeepapi/blob/fc21aad6f33b26f4f8c78117d2fca71f675763b6/gkeepapi/__init__.py#L933
https://github.com/kiwiz/gkeepapi/blob/fc21aad6f33b26f4f8c78117d2fca71f675763b6/gkeepapi/__init__.py#L811-L820
I think it should be Union[re.Pattern, str]
but I'm not sure
Got it. That sounds fine then. For re
, that block of code is indeed for Py2 support - Union[re.Pattern, str]
should do the trick with its removal.
I just realized typing.Optional
exists, so I'll be replacing every Union[X, None]
-> Optional[X]
In some places the args are optional but it's not mentioned in the docstrings, so I'll fix that too:
https://github.com/kiwiz/gkeepapi/blob/f0d066f8eda3492e9c884510265c3a2588123242/gkeepapi/__init__.py#L866-L871
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 X | Y
and Optionals with X | None
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 Makefile
in docs/
typing.Dict
instead of dict
for earlier Python version support 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 WARNING: duplicate object description of X...
Should be ready for review
Thanks for your work! I've merged the changes into deprecate_py2
. I'll do a review of the library and then merge into main.
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