Skip to content

Avoid omitting parentheses for trailing attributes on call expressions #6322

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.

Sign up for GitHub

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 1 commit into from
Aug 7, 2023
Merged

Avoid omitting parentheses for trailing attributes on call expressions #6322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/compare.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,55 @@
>= c
)
]

def f():
return (
unicodedata.normalize("NFKC", s1).casefold()
== unicodedata.normalize("NFKC", s2).casefold()
)

# Call expressions with trailing attributes.

ct_match = (
aaaaaaaaaaact_id == self.get_content_type(obj=rel_obj, using=instance._state.db).id
)

ct_match = (
{aaaaaaaaaaaaaaaa} == self.get_content_type(obj=rel_obj, using=instance._state.db).id
)

ct_match = (
(aaaaaaaaaaaaaaaa) == self.get_content_type(obj=rel_obj, using=instance._state.db).id
)

ct_match = aaaaaaaaaaact_id == self.get_content_type(
obj=rel_obj, using=instance._state.db
)

# Call expressions with trailing subscripts.

ct_match = (
aaaaaaaaaaact_id == self.get_content_type(obj=rel_obj, using=instance._state.db)[id]
)

ct_match = (
{aaaaaaaaaaaaaaaa} == self.get_content_type(obj=rel_obj, using=instance._state.db)[id]
)

ct_match = (
(aaaaaaaaaaaaaaaa) == self.get_content_type(obj=rel_obj, using=instance._state.db)[id]
)

# Subscripts expressions with trailing attributes.

ct_match = (
aaaaaaaaaaact_id == self.get_content_type[obj, rel_obj, using, instance._state.db].id
)

ct_match = (
{aaaaaaaaaaaaaaaa} == self.get_content_type[obj, rel_obj, using, instance._state.db].id
)

ct_match = (
(aaaaaaaaaaaaaaaa) == self.get_content_type[obj, rel_obj, using, instance._state.db].id
)
6 changes: 4 additions & 2 deletions crates/ruff_python_formatter/src/expression/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,10 +178,9 @@ impl Format<PyFormatContext<'_>> for MaybeParenthesizeExpression<'_> {
Parenthesize::Optional | Parenthesize::IfBreaks => needs_parentheses,
};

let can_omit_optional_parentheses = can_omit_optional_parentheses(expression, f.context());
match needs_parentheses {
OptionalParentheses::Multiline if *parenthesize != Parenthesize::IfRequired => {
if can_omit_optional_parentheses {
if can_omit_optional_parentheses(expression, f.context()) {
optional_parentheses(&expression.format().with_options(Parentheses::Never))
.fmt(f)
} else {
Expand Down Expand Up @@ -407,9 +406,12 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> {
attr: _,
ctx: _,
}) => {
self.visit_expr(value);
Copy link
Member

Choose a reason for hiding this comment

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

Can you test if we need the same for Subscript? Because it has the same pattern or is it not required because we never want to break subscripts?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did look into this but perhaps I didn't understand what you're referencing. Are you referring to a subscript with a trailing attribute, like foo[bar].baz? Or a subscript following a function call, like foo(bar)[baz]? I believe I added test coverage for both, and both are consistent with Black.

Copy link
Member

Choose a reason for hiding this comment

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

This PR fixes

ct_match = aaaaaaaaaaact_id == self.get_content_type(
    obj=rel_obj, using=instance._state.db
)

Where the last item is a call chain. The fix was to correctly set last to the call because the callee is to the left of the call. This fixed has_parentheses and has_own_parentheses to return true.

Now, we have a similar situation with subscript where we visit the value to the left of the subscript. Meaning a[b] ends with the subscript and not the node a (the subscript should be last).

Now, whether it should apply that layout is a different question, but it currently seems inconsistent because has_parentheses returns true, but we don't set the subscript to the last node.

Copy link
Member

Choose a reason for hiding this comment

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

Black output:

ct_match = (
    self.get_content_type["obj=rel_obj, using=instance._state.db"] == aaaaaaaaaaact_id
)

ct_match = (
    aaaaaaaaaaact_id == self.get_content_type["obj=rel_obj, using=instance._state.db"]
)

So black prefers not to break subscripts. Can we remove the Subscript case from has_own_parentheses?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will investigate, thank you for clarifying.

Copy link
Member

Choose a reason for hiding this comment

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

We can merge this in the meantime. Maybe update the title?

if has_parentheses(value, self.source) {
self.update_max_priority(OperatorPriority::Attribute);
}
self.last = Some(expr);
return;
}

Expr::NamedExpr(_)
Expand Down
106 changes: 106 additions & 0 deletions crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,58 @@ return 1 == 2 and (
>= c
)
]

def f():
return (
unicodedata.normalize("NFKC", s1).casefold()
== unicodedata.normalize("NFKC", s2).casefold()
)

# Call expressions with trailing attributes.

ct_match = (
aaaaaaaaaaact_id == self.get_content_type(obj=rel_obj, using=instance._state.db).id
)

ct_match = (
{aaaaaaaaaaaaaaaa} == self.get_content_type(obj=rel_obj, using=instance._state.db).id
)

ct_match = (
(aaaaaaaaaaaaaaaa) == self.get_content_type(obj=rel_obj, using=instance._state.db).id
)

ct_match = aaaaaaaaaaact_id == self.get_content_type(
obj=rel_obj, using=instance._state.db
)

# Call expressions with trailing subscripts.

ct_match = (
aaaaaaaaaaact_id == self.get_content_type(obj=rel_obj, using=instance._state.db)[id]
)

ct_match = (
{aaaaaaaaaaaaaaaa} == self.get_content_type(obj=rel_obj, using=instance._state.db)[id]
)

ct_match = (
(aaaaaaaaaaaaaaaa) == self.get_content_type(obj=rel_obj, using=instance._state.db)[id]
)

# Subscripts expressions with trailing attributes.

ct_match = (
aaaaaaaaaaact_id == self.get_content_type[obj, rel_obj, using, instance._state.db].id
)

ct_match = (
{aaaaaaaaaaaaaaaa} == self.get_content_type[obj, rel_obj, using, instance._state.db].id
)

ct_match = (
(aaaaaaaaaaaaaaaa) == self.get_content_type[obj, rel_obj, using, instance._state.db].id
)
```

## Output
Expand Down Expand Up @@ -171,6 +223,60 @@ return 1 == 2 and (
>= c
)
]


def f():
return unicodedata.normalize("NFKC", s1).casefold() == unicodedata.normalize(
"NFKC", s2
).casefold()


# Call expressions with trailing attributes.

ct_match = (
aaaaaaaaaaact_id == self.get_content_type(obj=rel_obj, using=instance._state.db).id
)

ct_match = {aaaaaaaaaaaaaaaa} == self.get_content_type(
obj=rel_obj, using=instance._state.db
).id

ct_match = (aaaaaaaaaaaaaaaa) == self.get_content_type(
obj=rel_obj, using=instance._state.db
).id

ct_match = aaaaaaaaaaact_id == self.get_content_type(
obj=rel_obj, using=instance._state.db
)

# Call expressions with trailing subscripts.

ct_match = (
aaaaaaaaaaact_id == self.get_content_type(obj=rel_obj, using=instance._state.db)[id]
)

ct_match = {
aaaaaaaaaaaaaaaa
} == self.get_content_type(obj=rel_obj, using=instance._state.db)[id]

ct_match = (
aaaaaaaaaaaaaaaa
) == self.get_content_type(obj=rel_obj, using=instance._state.db)[id]

# Subscripts expressions with trailing attributes.

ct_match = (
aaaaaaaaaaact_id
== self.get_content_type[obj, rel_obj, using, instance._state.db].id
)

ct_match = {
aaaaaaaaaaaaaaaa
} == self.get_content_type[obj, rel_obj, using, instance._state.db].id

ct_match = (
aaaaaaaaaaaaaaaa
) == self.get_content_type[obj, rel_obj, using, instance._state.db].id
```


Expand Down