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 --target-version
option to allow users to choose targeted Python versions
#618
Conversation
Pull Request Test Coverage Report for Build 896
💛 - Coveralls |
line_length: int = DEFAULT_LINE_LENGTH | ||
numeric_underscore_normalization: bool = True | ||
string_normalization: bool = True | ||
is_pyi: bool = False |
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.
Have you thought about folding is_pyi
into target_versions
?
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.
Yes, I'm not sure it's worth it though, since pyi mode is not quite like a Python version: you can't target say "pyi and CPython 3.6", it's always just pyi and nothing else.
"""Detect the version to target based on the nodes used.""" | ||
features = get_features_used(node) | ||
return { | ||
version for version in TargetVersion if features <= VERSION_TO_FEATURES[version] |
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.
Beautiful ❤️
blackd.py
Outdated
except (KeyError, ValueError): | ||
return web.Response( | ||
status=400, | ||
text=f"Invalid value for {PYTHON_VARIANT_HEADER}", |
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.
Let's have a more specific error message here. End users will likely encounter this if they misconfigure their clients
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.
Will do
blackd.py
Outdated
) | ||
for version in value.split(","): | ||
tag = "cpy" | ||
if version.startswith("cpy"): |
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 don't really like the explicit parsing here. Is it possible to try to match on all possible enum values 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.
Yes, it's not great. I tried to work with the following constraints:
- Continue accepting everything the previous code accepted (for backward compatibility). That's why there's the default if the user just put "3" or "2" as the version.
- Don't be too liberal in what I accept, because then future backward compatibility will bind us to continue accepting it.
- Avoid exposing our internal enum values too much. Users shouldn't have to care that we have a separate PyPy 3.5 version, but not one for other PyPy versions.
I'll see what I can change but I'm not sure there's much more I can do keeping these constraints.
There's some issue with asyncio in 3.8 that I don't fully understand now—I wonder whether it's even related to this diff, or whether it's just 3.8 instability. |
still trying to get a usable 3.8 build locally
The observed issue is an aiohttp bug. |
Thanks for confirming! I was thinking in the same direction. The reason it came up in this PR is that currently we actually skip all blackd tests because |
This generalizes the
--py36
flag to allow the user to specify exactly which Python versions should be supported by the code Black produces. For example, you can now sayblack -t cpy27 -t pypy35 setup.py
to produce code that is compatible with Python 2.7 and PyPy 3.5.Advantages of this approach include:
I haven't yet fully tested all the ways this could be used, but wanted to put it up to see if others (@ambv, @zsol) are OK with this approach.