gkeepapi icon indicating copy to clipboard operation
gkeepapi copied to clipboard

Add initial type hints

Open PrOF-kk opened this issue 3 years ago • 3 comments

Closes #118

  • Rebased deprecate_py2 onto master before adding type hints, so the diff and/or commit order might be wrong on GitHub until deprecate_py2 is also rebased (I think)
  • Added type hints to every public Keep method, except:
    • Any sync=True or similar bools, it's not needed
    • find and findLabel - 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 on

This doesn't address the update method shadowing on line 621, or the unused resync arguments

PrOF-kk avatar Aug 27 '22 18:08 PrOF-kk

Thanks for adding hints and cleaning up all the docstrings.

  • I can resolve any conflicts, so no need to worry about that
  • Could you elaborate on this? Are bools implicitly typed in some way?
  • IIRC, we can just reference re.Pattern now that we don't care about Python 2 (if you're specifically referring to the docstrings
  • I'll take a look at resync

Let me know if/once you're done with this PR and I can merge and do any cleanup.

kiwiz avatar Aug 27 '22 21:08 kiwiz

Sorry for the delay, I must've missed the notification.

  • Are bools implicitly typed in some way?

The (IDE/linter/type checker) should be able to already correctly guess the type for that: VSCode example
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

PrOF-kk avatar Sep 15 '22 15:09 PrOF-kk

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.

kiwiz avatar Sep 19 '22 14:09 kiwiz

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

PrOF-kk avatar Sep 26 '22 15:09 PrOF-kk

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/

kiwiz avatar Sep 26 '22 19:09 kiwiz

  • Force pushed to use typing.Dict instead of dict for earlier Python version support
  • Added regular expression query typing, removed Py2 support
  • Removed types in docstrings

PrOF-kk avatar Sep 27 '22 15:09 PrOF-kk

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

PrOF-kk avatar Oct 02 '22 11:10 PrOF-kk

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.

kiwiz avatar Oct 14 '22 15:10 kiwiz