Skip to content

Conversation

davidalber
Copy link
Contributor

@davidalber davidalber commented Feb 3, 2018

Configurations.md contains two snippets (here and here) whose format is not meant to verified. The current code in master tries and fails to format those snippets. This PR introduces a mechanism to opt configuration code snippets out of verification.

To opt a snippet out of verification, add // rustfmt: skip to the top of the snippet. This is visible when viewing the file through the web UI.

image

Here is the output of cargo test -- --ignored on master:

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

Here is the output of cargo test -- --ignored on the PR branch:

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

This is part of #1845.

@nrc
Copy link
Member

nrc commented Feb 4, 2018

is it possible to just use #![rustfmt_skip] instead of the special comment?

@davidalber
Copy link
Contributor Author

Correct me if I'm wrong, but from what I can see, #[rustfmt_skip] is done on single items or expressions or something. I'm trying to indicate the entire snippet is just not being processed at all, and I don't want to confuse readers by using #[rustfmt_skip] in a way that it doesn't really work in rustfmt.

I could use it on everything in the snippet that would actually need to have it, but that line of thinking led me to the realization that it will look strange to have that in the example and then in the next code block have code that was formatted (presumably omitting the #[rustfmt_skip]). That criticism also applies to what I've done in the PR, so I think it's not ideal.

What if we just introduce a new convention that does not use special directives that can be followed to not test a snippet? I'm thinking something close to what's already in Configurations.md. Currently, for the first part in question we have

### Example
Original Code:

```rust
fn foo() {
    println!("a");
}



fn bar() {
    println!("b");


    println!("c");
}
```

#### `1` (default):
```rust
fn foo() {
    println!("a");
}

fn bar() {
    println!("b");

    println!("c");
}
```

I suggest we change the first header to #### Example (before formatting):. Then the test framework can look for that and not try to format code blocks that come after it. Thoughts on that?

@nrc
Copy link
Member

nrc commented Feb 5, 2018

When you write #![...] that is an inner attribute and applies to the enclosing item, not the item it is written on. So using #![rustfmt_skip] should apply to the entire module (or crate) which is the snippet, i.e., you'd only need it once. Of course this depends a bit on the details of how you turn the snippet into a Rust program, but it should work.

@davidalber
Copy link
Contributor Author

Thanks for explaining!

You are right that #![rustfmt_skip] has the desired effect in snippets that are formatted. However, the snippets immediately below the ### Example in my previous post are not formatted. The configuration testing code treats a situation where it does not have a configuration option and value as an error. It was set up this strict way to reduce the chance for accidental introduction of snippets that are never tested, which is important because we'll never notice if something isn't tested.

Here are three ways we could get through this:

  1. We start testing all rust snippets. We don't consider situations where there's no configuration option and value an error. We just use the default configuration in that case. This could lead to snippets that are not being tested in the way that was intended. It also doesn't enforce the format in Configurations.md the way the test currently does.
  2. We almost continue what we're doing. Having a snippet without a configuration option and value is an error, unless there's a #![rustfmt_skip] on the first line of the snippet. Then we just format it using the default configuration. (Doing this, versus just skipping it in the test, still provides the service of complaining if the code has syntactic problems.)
  3. We do the thing I described in my previous post: have a standard way of expressing that a snippet shouldn't be formatted through the header above the snippet.

My favorite is Option 2.

@davidalber davidalber force-pushed the configuration-check-opt-out branch from b6adfa3 to e054b1d Compare February 5, 2018 17:06
@davidalber
Copy link
Contributor Author

I went ahead and preemptively implemented a draft Option 2. If we decide to go with something else, I'm happy to change it.

Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

I think this looks good. I have a nagging feeling that there might be some edge case where option 2 doesn't work, but lets do this for now, and deal with anything that comes up if/when it actually does.

However, this does need a rebase, sorry (and sorry it took a while to review - I was away from work most of last week).

@davidalber davidalber force-pushed the configuration-check-opt-out branch from e054b1d to 429dad7 Compare February 12, 2018 16:23
@davidalber
Copy link
Contributor Author

Thanks, @nrc! Rebased.

No worries about the review delay. Thank you for taking the time to look and discuss!

@nrc nrc merged commit 67d36c7 into rust-lang:master Feb 12, 2018
@davidalber davidalber deleted the configuration-check-opt-out branch February 13, 2018 08:39
@davidalber davidalber mentioned this pull request May 5, 2019
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