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 type hints #59

Merged
merged 9 commits into from
Jul 14, 2022
Merged

Add type hints #59

merged 9 commits into from
Jul 14, 2022

Conversation

MaxG87
Copy link
Contributor

@MaxG87 MaxG87 commented Dec 6, 2021

As announced in #52 this pull request adds type hints. It changes how certain functions have to be used, so it requires an increment of the major version, according to SemVer.

By adding type hints I made the existing complexity of pick explicit. I managed to simplify things by two changes:

  1. Always return a list of picks, except when a custom handler is used.
  2. Use a default options_map_func to get rid of a default type.

I adapted the tests and examples. I make use of my fork of pick in another project, mtv-cli. There it helped me to get rid of some type warnings, but it is not yet extensive use or battle tested or something.

@wong2
Copy link
Collaborator

wong2 commented Dec 6, 2021

Could you please rebase master?

The example in `options_map_func.py` shows that the intention of the
custom handler `options_map`func` is to make arbitrary objects
printable. Thus, the type annotations had to be extended to support this
usecase.
Instead of having an optional function the function is mandatory now but
has a default value. This allows to drop a case distinction and, much
more importantly, drop many of the alternatives of the type hints.
The examples still tried to unpack the returned list. This was not a
good demonstration of the new, streamlined, return style type.
@idoo
Copy link

idoo commented May 18, 2022

@wong2 could you pls merge it?

@MaxG87
Copy link
Contributor Author

MaxG87 commented Jul 9, 2022

Hi @wong2 , it seems as if this PR got stalled quite a bit. Is there anything I could do in order to get it merged?

@wong2 wong2 merged commit 1dedda4 into aisk:master Jul 14, 2022
@wong2
Copy link
Collaborator

wong2 commented Jul 14, 2022

Thanks @MaxG87

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.

3 participants