-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
CLN: Replaced "bool_t" with "builtins.bool" #32365
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
Conversation
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.
Thanks @MomIsBestFriend I like this, but the style guide recommends the alias approach. https://pandas.io/docs/development/contributing.html#style-guidelines
Do you know which approach is best practice?
@simonjayhawkins I have to be honest I didn't make this changes with Type annotations in mind, I simply saw And for your question, which approach is the best practice, I really don't know, is there anyone at |
looking at the solution in ahawker/ulid#26, does that look clearer? i.e. we define aliases in pandas._typing that use the same name with capitalization. so the alias for str is Str, bool is Bool etc. I also like this as it is consistent with dict and Dict (although for a different reason, Dict can accept type parameters) |
@@ -2608,7 +2606,7 @@ def to_pickle( | |||
to_pickle(self, path, compression=compression, protocol=protocol) | |||
|
|||
def to_clipboard( | |||
self, excel: bool_t = True, sep: Optional[str] = None, **kwargs | |||
self, excel: builtins.bool = True, sep: Optional[str] = None, **kwargs |
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.
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.
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.
Thanks. I recall a discussion about the added "clutter" to the docs with the addition of type annotations in the function signature. (which to be fair, is redundant since we add the types to the docstring)
These might not be required with recent updates to the semantic analyzer so can try that, but otherwise I don't think there's any real benefit to changing |
worth giving it a try, but we should probably keep the "alias trick" as "a good defensive solution" |
I agree with Will. The bool_t definition also has a comment telling us why it is there, which the builtins.bool doesnt |
I think that perhaps we could move the alias to pandas_typing and add a better explanation and ensure consistency. in in ahawker/ulid#26 the alias is in a shared hints module (equivalent to our pandas_typing) and looks like...
I think we should do that for bool here and str in the other two modules mentioned. ( and agree on _t vs _type vs Capitalization) |
Do we support |
#32449 should fix that. |
Would have to search but fairly certain what we have now is the suggested approach by Guido / mypy folks. Do any of these. alternatives have a clear benefit? If not I think should just close |
No, there is not a clear benefit. Closing, thank you all for the notes :) |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff