Skip to content

Getting binop_separator="Back" snippet in Configurations.md to pass #2361

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

Merged
merged 1 commit into from
Jan 16, 2018

Conversation

davidalber
Copy link
Contributor

As the next phase of #1845 I am fixing the formatting failures uncovered by #2292. Most of them are straightforward and will arrive as one big PR. I am submitting this one separately because it potentially exposes a bug.

In terms of getting the test to pass, here's the line failure count for cargo test -- --ignored before the change:

---- configuration_snippet_tests stdout ----
        Ran 104 configurations tests.
thread 'configuration_snippet_tests' panicked at 'assertion failed: `(left == right)`
  left: `55`,
 right: `0`: 55 configurations tests failed', tests/system.rs:790:5

Here's the line failure count for cargo test -- --ignored after the change:

---- configuration_snippet_tests stdout ----
        Ran 104 configurations tests.
thread 'configuration_snippet_tests' panicked at 'assertion failed: `(left == right)`
  left: `54`,
 right: `0`: 54 configurations tests failed', tests/system.rs:790:5

let range = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa..
bbbbbbbbbbbbbbbbbbbbbbbbbbbbb;
let range = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
..bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the part in question. The previous snipped had .. at the end of line. Currently, rustfmt with binop_separator="Back" places it on the next line. Is this expected?

Copy link
Member

Choose a reason for hiding this comment

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

No, this is a bug, I think, .. should respect the binop_separator option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Here's my thinking on this: I'd like to keep moving forward on the configuration snippets and create issues for bugs identified along the way. For this issue I've created #2364. There's three clear options:

  1. Leave the range notation line in this PR and add a comment pointing at Range notation not handled as expected for binop_separator="Back" configuration #2364. That's nice because when Range notation not handled as expected for binop_separator="Back" configuration #2364 is fixed, the configuration snippet will break, guaranteeing that it will get updated. The downside is that maybe you don't want bugs noted in the snippets.
  2. I remove the line in question from the snippet and make a note in Range notation not handled as expected for binop_separator="Back" configuration #2364 saying it should be added back before that issue is resolved.
  3. We could decide that finishing the configuration snippet changes before dealing with bugs identified isn't the way to go and that we should fix the bug before moving on. (It's also possible someone else will just fix it quickly.)

I'm going to update the PR to reflect Option 1 above. If you prefer a different option, I'll change it.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, option 1 sounds good!

Copy link
Member

Choose a reason for hiding this comment

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

I would use the format // FIXME(#1234) a short description of the bug for the comments. I'll merge this as is, but if you update it in a future PR, that would be good.

@nrc nrc merged commit 13a0bb9 into rust-lang:master Jan 16, 2018
@davidalber davidalber deleted the fix-binop-separator-back-snippet branch January 16, 2018 01:51
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.

2 participants