Skip to content
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

Merged
merged 8 commits into from
Oct 14, 2022
Merged

Add initial type hints #128

merged 8 commits into from
Oct 14, 2022

Conversation

PrOF-kk
Copy link
Contributor

@PrOF-kk PrOF-kk commented Aug 27, 2022

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

@kiwiz
Copy link
Owner

kiwiz commented Aug 27, 2022

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.

@PrOF-kk
Copy link
Contributor Author

PrOF-kk commented Sep 15, 2022

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?

try:
Pattern = re._pattern_type # pylint: disable=protected-access
except AttributeError:
Pattern = re.Pattern # pylint: disable=no-member

Which leads to not being sure what type to annotate query here and here:
def findLabel(self, query, create=False):

def find(
self,
query=None,
func=None,
labels=None,
colors=None,
pinned=None,
archived=None,
trashed=False,
): # pylint: disable=too-many-arguments

I think it should be Union[re.Pattern, str] but I'm not sure

@kiwiz
Copy link
Owner

kiwiz commented Sep 19, 2022

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.

@PrOF-kk
Copy link
Contributor Author

PrOF-kk commented Sep 26, 2022

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:

def createList(self, title=None, items=None):
"""Create a new list and populate it. Any changes to the note will be uploaded when :py:meth:`sync` is called.
Args:
title (str): The title of the list.
items (List[(str, bool)]): A list of tuples. Each tuple represents the text and checked status of the listitem.

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

@kiwiz
Copy link
Owner

kiwiz commented Sep 26, 2022

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/

Only user-facing methods have typing info for now
@PrOF-kk
Copy link
Contributor Author

PrOF-kk commented Sep 27, 2022

  • 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
Copy link
Contributor Author

PrOF-kk commented Oct 2, 2022

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

@kiwiz kiwiz merged commit ebc387e into kiwiz:deprecate_py2 Oct 14, 2022
@kiwiz
Copy link
Owner

kiwiz commented Oct 14, 2022

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.

@PrOF-kk PrOF-kk deleted the initialTyping branch October 14, 2022 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants