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

Work around stability errors due to optional trailing commas #2126

Merged
merged 2 commits into from Apr 25, 2021

Conversation

ambv
Copy link
Collaborator

@ambv ambv commented Apr 25, 2021

Optional trailing commas put by Black become magic trailing commas on another pass of the tool. Since they are influencing formatting around optional parentheses, on rare occasions the tool changes its mind in terms of putting parentheses or not.

Ideally this would never be the case but sadly the decision to put optional parentheses or not (which looks at pre-existing "magic" trailing commas) is happening around the same time as the decision to put an optional trailing comma. Untangling the two proved to be impractically difficult.

This shameful workaround uses the fact that the formatting instability introduced by magic trailing commas is deterministic: if the optional trailing comma becoming a pre-existing "magic" trailing comma changes formatting, the second pass becomes stable since there is no variable factor anymore on pass 3, 4, and so on.

For most files, this will introduce no performance penalty since --safe is already re-formatting everything twice to ensure formatting stability. We're using this result and if all's good, the behavior is equivalent. If there is a difference, we treat the second result as the binding one, and check its sanity again.

Optional trailing commas put by Black become magic trailing commas on another
pass of the tool.  Since they are influencing formatting around optional
parentheses, on rare occasions the tool changes its mind in terms of putting
parentheses or not.

Ideally this would never be the case but sadly the decision to put optional
parentheses or not (which looks at pre-existing "magic" trailing commas) is
happening around the same time as the decision to put an optional trailing
comma.  Untangling the two proved to be impractically difficult.

This shameful workaround uses the fact that the formatting instability
introduced by magic trailing commas is deterministic: if the optional trailing
comma becoming a pre-existing "magic" trailing comma changes formatting, the
second pass becomes stable since there is no variable factor anymore on pass 3,
4, and so on.

For most files, this will introduce no performance penalty since `--safe` is
already re-formatting everything twice to ensure formatting stability.  We're
using this result and if all's good, the behavior is equivalent.  If there is
a difference, we treat the second result as the binding one, and check its
sanity again.
@cooperlees cooperlees self-requested a review April 25, 2021 17:18
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 for submitting this! Hopefully we can get it in soon.

dst_contents = dst_contents_pass2
assert_equivalent(src_contents, dst_contents, pass_num=2)
assert_stable(src_contents, dst_contents, mode=mode)
# Note: no need to explicitly call `assert_stable` if `dst_contents` was
Copy link
Collaborator

Choose a reason for hiding this comment

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

We still need to call assert_equivalent though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We did, above in line 1018.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So we do. Sorry for the noise!

@jayaddison
Copy link
Contributor

@ambv Does this make it possible to remove the expectedFailure decorators here?

@ambv
Copy link
Collaborator Author

ambv commented Apr 25, 2021

No. Those are still failing. I added new tests that pass after two calls to format_str().

This was referenced Apr 25, 2021
This was referenced Apr 25, 2021
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.

None yet

3 participants