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

fix: Remove parenthesis around sole list items #4312

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

cobaltt7
Copy link
Contributor

@cobaltt7 cobaltt7 commented Apr 17, 2024

Fixes #3545, but it affects more places as well.

Description

I added this to the hug_parens_with_braces_and_square_brackets unstable style since there was a lot of overlapping code. I renamed it to concise_nested_brackets to reflect this. That is a breaking change if anyone was using --enable-unstable-feature hug_parens_with_braces_and_square_brackets, but to my knowlege, the existence of certain unstale features is not gurenteed to be stable. I can add the old name back as a (deprecated) alias or revert the rename altogether if so desired.

This does introduce more places where #4036 would be triggered:

items = [  # type: ignore
    (  # type: ignore
        {"key1": "val1", "key2": "val2", "key3": "val3"}
        if some_var == "longstring"
        else { "key": "val" }
    )
]

Checklist - did you ...

  • [y] Add an entry in CHANGES.md if necessary?
  • [y] Add / update tests if necessary?
  • [y] Add new / update outdated documentation?

Copy link

github-actions bot commented Apr 17, 2024

diff-shades reports zero changes comparing this PR (8c4519f) to main (b677a64).


What is this? | Workflow run | diff-shades documentation

Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks, these changes look good.

However, I think I'd prefer to keep feature names stable through the year. The main motivation for adding --enable-unstable-feature was to allow users who want to keep some feature on to just turn on the feature in their config and have it keep working.

Also, I don't want to accept a PR that directly adds a new unstable style. I see the unstable style as a holding pen for style changes that we'd originally put into preview but later ran into issues with. New style features should go into preview instead.

@cobaltt7
Copy link
Contributor Author

cobaltt7 commented Jun 7, 2024

Apologies for the delay @JelleZijlstra, I've had other priorities recently.

The main issue was hug_parens_with_braces_and_square_brackets was required for the parens to be removed around multi-line expressions. I tried to identify exactly what parts it depended on, but I don't think it would be an easy refactor. I settled on just documenting this limitation instead.

I put remove_lone_list_item_parens in the unstable style due to this limitation, although it does work on its own for simple cases. If desired, I could move it to the preview style so preview users could get the benefits in those cases, or we could wait for hug_parens_with_braces_and_square_brackets. The original issue #3545 was for the more complex cases.

@cobaltt7
Copy link
Contributor Author

cobaltt7 commented Oct 6, 2024

Hey @JelleZijlstra just returned to this PR, sorry for letting it go stale

I removed this change from the unstable style, because it doesn't add any issues on its own, it just doesn't work in all cases without hug_parens_with_braces_and_square_brackets. I also resolved the conflicts.

Do these changes look good?

Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thank you!

CHANGES.md Outdated Show resolved Hide resolved
@JelleZijlstra
Copy link
Collaborator

Maybe we should also extend this to single-element sets? The code should be very similar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A single conditional expression inside a list is unnecessarily parenthesized
2 participants