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

Remove for brackets: Extra unit test #2953

Closed
wants to merge 2 commits into from

Conversation

jpy-git
Copy link
Contributor

@jpy-git jpy-git commented Mar 24, 2022

Description

Investigating #2952. Really weird but I'm not seeing this issue and this unit test works fine for me 😅

Checklist - did you ...

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

@JelleZijlstra JelleZijlstra added the skip news Pull requests that don't need a changelog entry. label Mar 24, 2022
@ichard26
Copy link
Collaborator

Sigh, it turns out this is my fault as I had black's source code edited when reviewing this change (I was debugging another PR). This issue still exists, but it's hidden by the second pass we force:

black/src/black/__init__.py

Lines 1151 to 1157 in 14d84ba

dst_contents = _format_str_once(src_contents, mode=mode)
# Forced second pass to work around optional trailing commas (becoming
# forced trailing commas on pass 2) interacting differently with optional
# parentheses. Admittedly ugly.
if src_contents != dst_contents:
return _format_str_once(dst_contents, mode=mode)
return dst_contents

If you remove this hack for magic trailing commas then the instability shows up.

@jpy-git
Copy link
Contributor Author

jpy-git commented Mar 24, 2022

If you remove this hack for magic trailing commas then the instability shows up.

Ah thanks was wondering why I wasn't seeing the issue!

@ichard26
Copy link
Collaborator

I'd be fine with relying on this hack for now, but we want to remove it eventually so that probably just kicks the can down the road.

@jpy-git
Copy link
Contributor Author

jpy-git commented Mar 24, 2022

Makes sense, I've added a note to this unit test so at least we know the hack works for now. Will aim to fix this as part of #2926 since I'm pretty sure it'll be the same issue!

@jpy-git
Copy link
Contributor Author

jpy-git commented Mar 25, 2022

Going to close this as I've figured out how to fix this for both the for and with case and will address this as part of #2926 😄

@jpy-git jpy-git closed this Mar 25, 2022
@jpy-git jpy-git deleted the for_multi_brackets_test branch March 25, 2022 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news Pull requests that don't need a changelog entry.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants