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

Is --target-version the minimum or should all be explicitly listed? #751

Open
hugovk opened this issue Mar 14, 2019 · 16 comments
Open

Is --target-version the minimum or should all be explicitly listed? #751

hugovk opened this issue Mar 14, 2019 · 16 comments
Labels
C: configuration CLI and configuration T: documentation Improvements to the docs (e.g. new topic, correction, etc)

Comments

@hugovk
Copy link
Contributor

hugovk commented Mar 14, 2019

With old Black 18.9b0, I had this in pyproject.toml:

[tool.black]
py36 = true

The new Black 19.3b0 warns:

--py36 is deprecated and will be removed in a future version. Use --target-version py36 instead.

The docs say:

  -t, --target-version [py27|py33|py34|py35|py36|py37|py38]
                                  Python versions that should be supported by
                                  Black's output. [default: per-file auto-
                                  detection]
  --py36                          Allow using Python 3.6-only syntax on all
                                  input files.  This will put trailing commas
                                  in function signatures and calls also after
                                  *args and **kwargs. Deprecated; use
                                  --target-version instead. [default: per-file
                                  auto-detection]

I see Black's own pyproject.toml includes:

[tool.black]
target_version = ['py36', 'py37', 'py38']

If I target just py36, does it treat that as a minimum?

Is that the same as ['py36', 'py37', 'py38']?

It recommended to explicitly list all the targetted versions?

@JelleZijlstra
Copy link
Collaborator

You should include all Python versions that you want your code to run under. In practice, though, 3.6 through 3.8 are currently the same as far as Black is concerned, so either variant will do the same.

Let us know if you think the docs can be improved.

@ambv
Copy link
Collaborator

ambv commented Mar 14, 2019

Jelle, I get that certain edge cases in the grammar are only available in the newer versions, so we need the lower bound. Why do we need the upper bound, too?

@hynek
Copy link
Contributor

hynek commented Mar 16, 2019

FWIW I just had to look into black's source cod too, to figure out how to set target-version.

From the docs it's unclear that it takes multiple versions and reusing --target-version=py36 as target-version=py36 fails with confusing errors.

I think it's fine to allow supplying a range (although I'd prefer something like py27...py38 or py37... that conveys the meaning better), but the docs don't reflect that currently.

@JelleZijlstra JelleZijlstra added the C: configuration CLI and configuration label May 5, 2019
@hugovk
Copy link
Contributor Author

hugovk commented Nov 1, 2019

Another confusion.

Let's say I support Python 3.5-3.8 so have this in pyproject.toml:

[tool.black]
target_version = ['py35', 'py36', 'py37', 'py38']

If I don't have this config file and want to do the same thing directly on the command line, do I need to repeat each argument?

$ black --target-version py35 --target-version py36 --target-version py37 --target-version py38 .

And then would .pre-commit.yaml need to look like this?

repos:
  - repo: https://github.com/psf/black
    rev: 19.10b0
    hooks:
      - id: black
        args: ["--target-version", "py35", "--target-version", "py36", "--target-version", "py37", "--target-version", "py38"]

These are pretty long, and it'd be nice to have a shorter range argument (or just a single lower bound).


I'm also curious why we need the upper bound. In practice, 3.5-3.8 is currently the same as just 3.5. What sort of theoretical cases are there where it'd be different?

Thanks!

@JelleZijlstra
Copy link
Collaborator

I'm also curious why we need the upper bound. In practice, 3.5-3.8 is currently the same as just 3.5. What sort of theoretical cases are there where it'd be different?

In theory, a new version of Python could change the syntax such that a kind of formatting that Black thinks is ideal is no longer supported. As a hypothetical example, maybe Python 3.9 will decide that all string prefixes have to be uppercase, so f"{x}" will be SyntaxError and only F"{x}" will be legal. In that case, if you set target_version to 3.5-3.8, we'd still emit f"{x}" (because Black's style prefers lowercase string prefixes), but with 3.5-3.9 we'd have to emit F"{x}".

In practice that sort of scenario doesn't seem terribly likely, so I'd be OK with making target_version just a minimum requirement.

@Zac-HD
Copy link
Contributor

Zac-HD commented Dec 29, 2019

In practice that sort of scenario doesn't seem terribly likely, so I'd be OK with making target_version just a minimum requirement.

I was very surprised to discover it wasn't already a minimum requirement, and that only happened when I stumbled across this issue!

FWIW I think making forward-compatibilty the easy option is also better for the ecosystem 😄

@ambv
Copy link
Collaborator

ambv commented Mar 4, 2020

At the same time I agree that it should almost certainly be 3.6+, 3.7+, 3.8+, and so on. But Jelle's got a point about possible syntax breaking changes in the future. I'm not sure if we should make this change.

@Zac-HD
Copy link
Contributor

Zac-HD commented Mar 5, 2020

I'd hope the answer is "Python 2 taught us not to make breaking changes to the syntax" - and if we do, adding an optional --maximum-version argument will be the least of the problems dumped on the library ecosystem.

@henryiii
Copy link
Contributor

My suggestion would be to read project.requires-python from pyproject.toml; you can compare using packaging.specifiers to see what the allowed versions are. cibuildwheel 1.9 does this. Since PEP 621, that is the canonical place for this information, and powerful enough to provide a subset or limit if needed.

See https://github.com/joerick/cibuildwheel/blob/58c5e56e9f271e32020b687f62cc02003602edd2/cibuildwheel/__main__.py#L173 and https://github.com/joerick/cibuildwheel/blob/58c5e56e9f271e32020b687f62cc02003602edd2/cibuildwheel/util.py#L77

@deathaxe
Copy link

if you set target_version to 3.5-3.8, we'd still emit f"{x}" (because Black's style prefers lowercase string prefixes), but with 3.5-3.9 we'd have to emit F"{x}".

This doesn't apply for python 3.3 does it? Black still outputs f-strings even though they are not supported.

@JelleZijlstra
Copy link
Collaborator

This doesn't apply for python 3.3 does it

Black doesn't support Python 3.3. Also, note that you're quoting a hypothetical out of context.

@scottclowe
Copy link

@hugovk

And then would .pre-commit.yaml need to look like this?

repos:
  - repo: https://github.com/psf/black
    rev: 19.10b0
    hooks:
      - id: black
        args: ["--target-version", "py35", "--target-version", "py36", "--target-version", "py37", "--target-version", "py38"]

These are pretty long, and it'd be nice to have a shorter range argument (or just a single lower bound).

You can alternatively write the yaml with the arguments stacked in a vertical list instead, which I think is a lot easier to read when you're specifying more than three versions for black to target.

repos:
  - repo: https://github.com/psf/black
    rev: 19.10b0
    hooks:
      - id: black
        args:
            - "--target-version=py35"
            - "--target-version=py36"
            - "--target-version=py37"
            - "--target-version=py38"

graingert added a commit to graingert/aiometer that referenced this issue Jul 4, 2021
florimondmanca pushed a commit to florimondmanca/aiometer that referenced this issue Jul 5, 2021
* port to anyio v3

* configure strict pytest

* support py3.6 and py3.9

* add all black target-versions

psf/black#751 (comment)

* upgrade florimondmanca/azure-pipelines-templates

* upgrade black

* add nocov

* widen typing-extensions dep

* docment 3.6 support

* drop aiometer._concurrency
facebook-github-bot pushed a commit to pytorch/captum that referenced this issue Mar 22, 2022
Summary:
according psf/black#751 , set the minimum target-version of black to py36 & fix the style issues

Pull Request resolved: #896

Reviewed By: vivekmig

Differential Revision: D35038239

Pulled By: aobo-y

fbshipit-source-id: 7dd76d35cace8aaa286eebb092f038bb0f2a120d
@ichard26
Copy link
Collaborator

@JelleZijlstra and I have decided to make --target-version the minimum. It better fits user expectations and it's the only way to reasonably support #3124. We'll still need to decide how specifying a single vs multiple versions will work though.

@yilei
Copy link
Contributor

yilei commented Jan 20, 2023

While implementing #3489, I came up with another need: a project may want to set a minimum version but still allow auto detecting target version when it uses newer syntaxes from later versions.

A concrete example: a projects supports Python 3.9+, but it may still contain some files with pattern matching syntax (a common case is its test files, it wants to make sure it works with pattern matching, but the test only runs on Python 3.10+).

Without specifying a minimum version of 3.9, it can't use parenthesized context managers (when the file doesn't contain syntaxes from 3.9+). But setting --target-version=3.9 today would make it fail to parse 3.10+ syntaxes.

@itdependsnetworks
Copy link

@JelleZijlstra and I have decided to make --target-version the minimum. It better fits user expectations and it's the only way to reasonably support #3124. We'll still need to decide how specifying a single vs multiple versions will work though.

Is it safe to say that thought it has been decided and has not been implemented? My interpretation of this thread is that current commendation for py38-py311 support would be:

target-version = ['py38', 'py39', 'py310', 'py311']

In the future it would be equivalent of:

target-version = ['py38']

@adamjstewart
Copy link

Any updates on this thread? I would also prefer specifying only the minimum version as it doesn't require using a bleeding edge version of black just to have it recognize 'py312' as a valid target version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: configuration CLI and configuration T: documentation Improvements to the docs (e.g. new topic, correction, etc)
Projects
None yet
Development

No branches or pull requests