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

Strip even more redundant for parentheses #3243

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ichard26
Copy link
Collaborator

@ichard26 ichard26 commented Aug 29, 2022

Description

Fixes #3080.

What remains is removing unnecessary brackets because yes, you can do this:

for [x, y] in points:
    print("this is valid code!!!")

I'm leaving this as a future task because it turns out simply converting these square brackets into parentheses is not an AST-safe transformation so letting the pre-existing logic handle the rest doesn't work. And maybe_make_parens_invisible_in_atom is currently not designed to handle square brackets and initial attempts to add a boolean treat_square_brackets_as_parens parameter were unsuccessful.

Checklist - did you ...

  • Add a CHANGELOG entry if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

What remains is removing unnecessary brackets because yes, you can do
this:

    for [x, y] in points:
        print("this is valid code!!!")

I'm leaving this as a future task because it turns out simply
converting these square brackets into parentheses is not an AST-safe
transformation. And`maybe_make_parens_invisible_in_atom` is currently
not designed to handle square brackets and initial attemps to add a
boolean `treat_square_brackets_as_parens` parameter were unsuccessful.
@ichard26 ichard26 added the F: parentheses Too many parentheses, not enough parentheses, and so on. label Aug 29, 2022
@ichard26 ichard26 marked this pull request as ready for review August 29, 2022 00:02
@github-actions
Copy link

diff-shades reports zero changes comparing this PR (31d0d26) to main (c0cc19b).


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! I wouldn't worry about square brackets; that's a different and more rarely used syntax that we can handle later if we choose to make more AST-unsafe changes.

) in points:
pass

for one_super_long_name_as_the_for_target_list_because_why_not_you_dont_control_me_i_have_total_freedom in (points):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The odd parentheses is a pre-existing bug in main. Something in right_hand_split or something similar is deciding to make the invisible parentheses around points visible even though the split failed. I'm happy to try to fix this before merging, but I probably won't get to it until September.

@ichard26
Copy link
Collaborator Author

diff-shades is timing out, it could be simply GHA flakiness, but there could be a genuine infinite loop (or massive slowdown) somewhere. Since I'm in no rush to land this I'll mark this as draft.

@ichard26 ichard26 marked this pull request as draft August 31, 2022 16:53
@ichard26 ichard26 added this to the Release 23.2.0 milestone Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F: parentheses Too many parentheses, not enough parentheses, and so on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unnecessary parentheses in unpacking aren't removed
2 participants