-
Notifications
You must be signed in to change notification settings - Fork 54
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
MNT Add light type annotations to skops.io #219
Merged
adrinjalali
merged 8 commits into
skops-dev:main
from
BenjaminBossan:audit-type-annotations
Dec 1, 2022
Merged
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
2279172
Add light type annotation to skops.io
BenjaminBossan 1d610f9
Python 3.7: Import Literal from fixes
BenjaminBossan 975283d
Update skops/io/_audit.py
BenjaminBossan b984727
Type annotation: constructor can be a function too
BenjaminBossan b790323
Add returns part of get_tree docstring
BenjaminBossan 8da4bdd
Fix an error in persistence documentation
BenjaminBossan 662b593
Remove Any return type annotation from _construct
BenjaminBossan 9358f36
Add comment explaining Geneator type annotation
BenjaminBossan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does
Generator[None, None, None]
mean? typehints should helps us understand the code better, not to make them more confusing :DAlso, if everything is
Any
here, then why do we have typehints?This is an example where I wouldn't add any typehints here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The meaning for
Generator
isGenerator[YieldType, SendType, ReturnType]
, which happens to beNone, None, None
here. I agree that types don't add much here, though**kwargs: Any
is a shorthand fordict[str, Any]
. The difference for me between a function annotated withAny
vs no annotation is that with the latter, I don't know if it takes any or if it was just not annotated yet, so I guess not completely useless.I would thus keep the annotations here, but if you insist, I can remove them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think typehints should be useful, if they're not, they take extra space and make things less readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Adrin
imo, type hints should really be used like docstrings, if having them just takes up space and doesn't make things clearer or help an IDE, they probably don't need to be there.