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

CLN: Replaced "bool_t" with "builtins.bool" #32365

Closed
wants to merge 3 commits into from

Conversation

ShaharNaveh
Copy link
Member

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@simonjayhawkins simonjayhawkins added the Typing type annotations, mypy/pyright type checking label Feb 29, 2020
Copy link
Member

@simonjayhawkins simonjayhawkins left a 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?

@ShaharNaveh
Copy link
Member Author

@simonjayhawkins I have to be honest I didn't make this changes with Type annotations in mind, I simply saw bool_t and didn't knew what it was, and thought this changes will not cause future confusion.

And for your question, which approach is the best practice, I really don't know, is there anyone at mypy that we can ask?

@simonjayhawkins
Copy link
Member

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)

@WillAyd

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when rendering the docs, the aliased type is displayed.

image

can you render the docs for these changes, so we can see the output.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs looks the same to me:

builtins_bool_doc

Copy link
Member

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)

@WillAyd
Copy link
Member

WillAyd commented Feb 29, 2020

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

@simonjayhawkins
Copy link
Member

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"

@jbrockmendel
Copy link
Member

I agree with Will. The bool_t definition also has a comment telling us why it is there, which the builtins.bool doesnt

@simonjayhawkins
Copy link
Member

I think that perhaps we could move the alias to pandas_typing and add a better explanation and ensure consistency.

in pandas\core\indexes\base.py we have str_t = str and in pandas\core\dtypes\dtypes.py we have str_type = str, neither of which have comments to explain.

in ahawker/ulid#26 the alias is in a shared hints module (equivalent to our pandas_typing)

and looks like...

#: Type hint that is an alias for the built-in :class:`~str` type.
Str = str

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)

@ShaharNaveh
Copy link
Member Author

Do we support float16?

@simonjayhawkins
Copy link
Member

Do we support float16?

#32449 should fix that.

@WillAyd
Copy link
Member

WillAyd commented Mar 11, 2020

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

@ShaharNaveh
Copy link
Member Author

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants