Skip to content

Always use bool and Literal for Python compat code #9213

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

Merged
merged 3 commits into from
Nov 16, 2022

Conversation

sobolevn
Copy link
Member

When values used for python compat are defined, they are always bool.
But, we can also highlight that some versions are not supported at all: by marking them as Literal[False].

@AlexWaygood
Copy link
Member

Some of these go against the policy we established in #7258

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@sobolevn
Copy link
Member Author

sobolevn commented Nov 16, 2022

@AlexWaygood I don't think it is:

  1. There are exactly the same annotations we have in multiple other places
  2. These are real Literal constants that affect code reachability, that's their main goal

@AlexWaygood
Copy link
Member

  1. There are exactly the same annotations we have in multiple other places

We agreed in #7258 that we'd pause converting global constants to Literal types. We didn't say that we wanted to revert all of the global constants that already use Literal types. That's going to necessarily lead to a certain amount of inconsistency.

  1. These are real Literal constants that affect code reachability, that's their main goal

I don't have a problem with these constants being included in the stubs, since it's plausible that some users might be using them and/or find them useful, and I don't think they do much harm. But I think they're mostly intended as internal implementation details rather than things that are meant to be publicly exported.

@sobolevn
Copy link
Member Author

But I think they're mostly intended as internal implementation details rather than things that are meant to be publicly exported.

It depends, at least 3rd-party plugins for sqlalchemy and requests might be using them to have similar features support for different languages.

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 16, 2022

But I think they're mostly intended as internal implementation details rather than things that are meant to be publicly exported.

It depends, at least 3rd-party plugins for sqlalchemy and requests might be using them to have similar features support for different languages.

That's an interesting point. Are 3rd-party plugins for requests and sqlalchemy a thing?

It's worth noting that neither requests nor sqlalchemy appear to document these constants anywhere, so if third-party plugins are using them, they probably shouldn't be.

@AlexWaygood AlexWaygood requested a review from srittau November 16, 2022 16:52
Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

I don't think the reasoning from #7258 applies here. From rereading that issue, the main problem was "enum-like" literals. When assigning such a literal to a field or variable and later re-assigning the variable, type checkers will complain. I can't really imagine situations where you would want to assign these "flag-like" literals and later change the flag.

@AlexWaygood
Copy link
Member

Alrighty, let's do it then

@AlexWaygood AlexWaygood merged commit c626137 into python:main Nov 16, 2022
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