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: Break quality check before breaking parentheses #6065

Closed
Tracked by #6069 ...
konstin opened this issue Jul 25, 2023 · 2 comments · Fixed by #6322
Closed
Tracked by #6069 ...

Formatter: Break quality check before breaking parentheses #6065

konstin opened this issue Jul 25, 2023 · 2 comments · Fixed by #6322
Assignees
Labels
formatter Related to the formatter

Comments

@konstin
Copy link
Member

konstin commented Jul 25, 2023

We need to change the breaking priorities with equality checks and parentheses:

black:

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

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

ours:

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

ct_match = aaaaaaaaaaact_id == self.get_content_type(
    obj=rel_obj, using=instance._state.db
).id
@konstin konstin added the formatter Related to the formatter label Jul 25, 2023
@MichaReiser
Copy link
Member

MichaReiser commented Jul 25, 2023

There could be two possible solutions for this

  • Either can_omit_parentheses incorrectly returns false
    Or call expressions should not use parenthesized (which gives them a higher priority)

Or it's something entirely different

We may want to rename the issue. This is specific to call expressions. Lists, tuples should have a higher priority

@MichaReiser MichaReiser added this to the Formatter: Alpha milestone Jul 31, 2023
@charliermarsh
Copy link
Member

This is pretty specific -- it seems like it has to be a comparator, with a call on either side, and without another parenthesized node? E.g., Black formats this:

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

As:

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

@charliermarsh charliermarsh self-assigned this Aug 3, 2023
charliermarsh added a commit that referenced this issue Aug 7, 2023
#6322)

## Summary

This PR modifies our `can_omit_optional_parentheses` rules to ensure
that if we see a call followed by an attribute, we treat that as an
attribute access rather than a splittable call expression.

This in turn ensures that we wrap like:

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

For calls, but:

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

For calls with trailing attribute accesses.

Closes #6065.

## Test Plan

Similarity index before:

- `zulip`: 0.99436
- `django`: 0.99779
- `warehouse`: 0.99504
- `transformers`: 0.99403
- `cpython`: 0.75912
- `typeshed`: 0.72293

And after:

- `zulip`: 0.99436
- `django`: 0.99780
- `warehouse`: 0.99504
- `transformers`: 0.99404
- `cpython`: 0.75913
- `typeshed`: 0.72293
durumu pushed a commit to durumu/ruff that referenced this issue Aug 12, 2023
astral-sh#6322)

## Summary

This PR modifies our `can_omit_optional_parentheses` rules to ensure
that if we see a call followed by an attribute, we treat that as an
attribute access rather than a splittable call expression.

This in turn ensures that we wrap like:

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

For calls, but:

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

For calls with trailing attribute accesses.

Closes astral-sh#6065.

## Test Plan

Similarity index before:

- `zulip`: 0.99436
- `django`: 0.99779
- `warehouse`: 0.99504
- `transformers`: 0.99403
- `cpython`: 0.75912
- `typeshed`: 0.72293

And after:

- `zulip`: 0.99436
- `django`: 0.99780
- `warehouse`: 0.99504
- `transformers`: 0.99404
- `cpython`: 0.75913
- `typeshed`: 0.72293
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

Successfully merging a pull request may close this issue.

3 participants