Python: move unary operator spacing into SpacesVisitor - #8543
Open
knutwannheden wants to merge 3 commits into
Open
Python: move unary operator spacing into SpacesVisitor#8543knutwannheden wants to merge 3 commits into
knutwannheden wants to merge 3 commits into
Conversation
`MinimumViableSpacingVisitor` added a space between `not` and its operand whenever the operand prefix was empty, which is a style decision rather than a syntactic requirement: `not(x)`, `not[1]` and `not'x'` are all legal. That made auto-format rewrite `not(x)` to `not (x)` in files no recipe had touched, with nothing downstream reconciling it. `SpacesVisitor` now owns unary spacing, matching the Java formatter's split between the two visitors. `not` always gets a separator and the punctuation operators are tightened, which is what PEP 8, black and PyCharm produce; the Python `SpacesStyle` gains no new field, mirroring PyCharm's Python settings, which expose no unary option. `MinimumViableSpacingVisitor` now adds a space only where a keyword and the following expression would lex as a single token, and applies that rule to the expression side of `assert`, `return`, `and`/`or` and the `if`/`else` of a conditional expression as well. A recipe that wraps an `assert` condition in `not` without transferring the condition's prefix previously printed `assertnot x`.
`in`, `not in`, `is` and `is not` are keyword operators on `Py.Binary`, so a right operand with an empty prefix prints as `x iny`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
not#8540 taughtMinimumViableSpacingVisitorto put a space betweennotand its operand whenever the operand's prefix was empty. That space is a style decision rather than a syntactic requirement —not(x),not[1]andnot'x'are all legal Python — andMinimumViableSpacingVisitorruns over the whole compilation unit insideAutoFormat, so auto-formatting rewrotenot(x)tonot (x)in files no recipe had touched, with nothing downstream reconciling it.The Java formatter already draws this line:
AutoFormatVisitorrunsMinimumViableSpacingVisitorand thenSpacesVisitor, andSpacesVisitor.visitUnarynormalizes the operand prefix per style. Python had neither half —SpacesVisitorhas novisitUnary, so whatever the minimum-spacing pass produced was final.The same pass was also missing the other side of the problem. A recipe that wraps an
assertcondition innotwithout transferring the condition's prefix printedassertnot x, and auto-format left it that way.Examples
SpacesVisitornow owns unary spacing, which for Python meansnotalways gets a separator and+,-and~are tightened — what PEP 8, black and PyCharm all produce:MinimumViableSpacingVisitoradds a space only where a keyword and the expression that follows it would lex as a single token, so on its own it leaves both of those alone. Given a recipe that buildsnotwith empty prefixes, it now produces:The last two differ because the f-string's source starts with an identifier character.
Summary
SpacesVisitor.visit_unarynormalizes the operand prefix: one space afternot, none after+,-and~. No newSpacesStylefield — Python'sSpacesStyle.AroundOperatorsmirrors PyCharm's Python settings, which expose no unary option, andand/oralready hardcode their separator the same way.MinimumViableSpacingVisitordecides by printing the expression and testing its first character, so the rule tracks what is actually emitted rather than a list of node types. The empty-prefix guard runs first, so parsed code essentially never reaches the print call.assert,return,and/or,in/not in/is/is not, and both theifandelseof a conditional expression.ifandwhileconditions need nothing: they are wrapped inControlParentheses, which keeps the space after the keyword when a recipe replaces the inner tree.Two things are deliberately left out. The mirror direction — an expression immediately before a keyword, e.g.
a if cif a recipe clearedpadding.true_part.before— needs the last printed character of the left side and has no failing case yet. So do the remaining keyword parents (del,raise,yield,await,lambda,global,import/as), each a few lines with the helper now in place.Test plan
tests/python/all/format/spaces_test.py— new file coveringnotgaining a space, collapsingnot x, and tightening- zand~ ztests/python/all/format/minimum_viable_spacing_test.py—not(x)andnot[1]unchanged; generatednotseparated from an identifier and an f-string but not from(,[or a string literal; generated operands separated fromassert,return,and,inand theif/elseof a conditional expressionpytest tests --ignore=tests/rpc— 1933 passed, 61 skipped