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

Formatter: Target version support #7234

Closed
3 tasks
MichaReiser opened this issue Sep 8, 2023 · 10 comments
Closed
3 tasks

Formatter: Target version support #7234

MichaReiser opened this issue Sep 8, 2023 · 10 comments
Assignees
Labels
formatter Related to the formatter

Comments

@MichaReiser
Copy link
Member

MichaReiser commented Sep 8, 2023

Adjust the formatting based on the syntax supported by the targeted python versions.

Black uses this option to decide what grammar to use to parse your code. In addition, it may use it to decide what style to use. For example, support for a trailing comma after *args in a function call was added in Python 3.5, so Black will add this comma only if the target versions are all Python 3.5 or higher.

Source

  • Decide on minimal supported python version
  • Trailing args and kwargs comments (depending on the above decision)
  • Parenthesized with items

Relevant features:

@MichaReiser MichaReiser added the formatter Related to the formatter label Sep 8, 2023
@MichaReiser MichaReiser added this to the Formatter: Beta milestone Sep 8, 2023
@konstin
Copy link
Member

konstin commented Sep 12, 2023

Do we have a list of syntax that would change formatter behaviour that is not in 3.7/3.8? I can only think of nested f-string quotes in 3.12

@MichaReiser
Copy link
Member Author

Do we have a list of syntax that would change formatter behaviour that is not in 3.7/3.8? I can only think of nested f-string quotes in 3.12

f-strings will be fun because they now can contain comments too, yeah.

I don't have more than what's outlined in the issue above and is derived from Black's documentation. We could do a quick search on Black's repository to identify other version-specific formatting (or look at their version-specific tests)

@charliermarsh
Copy link
Member

Parenthesized with items (mentioned above) -- need to make sure we don't introduce parentheses there if unsupported. I don't know if this needs changes or not, but it was new in Python 3.10. (They have crates/ruff_python_formatter/resources/test/fixtures/black/py_39/remove_with_brackets.py for this, I think?)

See also crates/ruff_python_formatter/resources/test/fixtures/black/py_39/pep_572_py39.py -- walrus operators can be unparenthesized in some cases starting in Python 3.9, so we need to make sure we don't remove parentheses in such cases on older versions.

@MichaReiser
Copy link
Member Author

MichaReiser commented Sep 13, 2023

For my projects, the quote normalization would be OK, as this should be already fixed by the Q rules in the linter. However, the prefix normalization would be very problematic, as it breaks backwards compatibility with some older versions of Pythons that we still need to support for a couple of our projects. Specifically, the issue is the change from br"..." to rb"..." for binary regular expressions, which causes a SyntaxError. Without some type of replacement for this --skip-string-normalization option, we would not be able to switch to Ruff for formatting.

Originally answer by @tdulcet

Not versions that are supported by Ruff. Specifically, Python 3.1 and older, including 2.7 which we still need to support unfortunately... [[source](Not versions that are supported by Ruff. Specifically, Python 3.1 and older, including 2.7 which we still need to support unfortunately...)]

@charliermarsh
Copy link
Member

I've confirmed that the unparenthesized walruses is handled correctly (we never remove those parentheses, so we're fine).

@charliermarsh
Copy link
Member

We also get the remove_with_brackets.py test case right. I've confirmed that we only add parentheses in a with if there were already parentheses to start. (Can see should_parenthesize in stmt_with.rs; the other case is that in which there's a parenthesized dangling comment, which again implies parentheses.)

@charliermarsh
Copy link
Member

As far as I can tell, there's nothing that we need to do here, assuming...

  1. We only want to support Python 3.7 and later.
  2. We don't care about downgrading code (e.g., if code was written for Python 3.10, and uses a parenthesized with, then when run under --target-version py37, we consider it validate to retain the parenthesized with).

Some notes on Black's behavior:

  • I believe that Black adheres to (2) -- so if you run over a file with a match statement using --target-version py37, Black will fail. Black uses the target version to determine which parser to use.
  • Black has a VERSION_TO_FEATURES concept whereby it detects features that are used in a given file, and uses that to infer the target version, if it's not set (see: detect_target_versions).
  • After inferring the target versions, the only version-specific features that affect formatting are Feature.TRAILING_COMMA_IN_CALL and Feature.TRAILING_COMMA_IN_DEF (which apply for Python 3.5 and later, i.e., the versions we support), and Feature.PARENTHESIZED_CONTEXT_MANAGERS.

This last piece (Feature.PARENTHESIZED_CONTEXT_MANAGERS) is interesting, but is only relevant for preview style: https://black.readthedocs.io/en/stable/the_black_code_style/future_style.html#using-backslashes-for-with-statements. In short, Black is going to wrap multiple with statements using backslashes on older versions or parentheses on newer versions. Right now, it seems like the parenthesized version is implemented in preview mode. So, once we do preview style, we'll need to support that and gate it on target version.

For example, given:

with a as b, c as d, e(1,) as f:
    pass

Black stable formats as:

with a as b, c as d, e(
    1,
) as f:
    pass

But preview gives you:

with (
    a as b,
    c as d,
    e(
        1,
    ) as f,
):
    pass

@charliermarsh charliermarsh self-assigned this Sep 14, 2023
@MichaReiser
Copy link
Member Author

(Can see should_parenthesize in stmt_with.rs; the other case is that in which there's a parenthesized dangling comment, which again implies parentheses.)

This may no longer be true if we implement #7243

We don't care about downgrading code (e.g., if code was written for Python 3.10, and uses a parenthesized with, then when run under --target-version py37, we consider it validate to retain the parenthesized with).

I'm unsure whether I would expect my formatter to downgrade the syntax. I guess mainly because I'm just not used to having this setting in a formatter. Removing parentheses raises interesting question when it comes to comments, especially leading and trailing commnts

@MichaReiser
Copy link
Member Author

What's the status on this? Do we plan to support this for the Beta?

@charliermarsh
Copy link
Member

My assessment is that there's nothing to do here until we support preview style.

charliermarsh added a commit that referenced this issue Oct 19, 2023
## Summary

This doesn't affect behavior _yet_ (see:
#7234), but it will be needed in
the future, and it's surprising to users that it doesn't exist.

Closes #8051.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter
Projects
None yet
Development

No branches or pull requests

3 participants