Narrowing down rustfmt skip yields unformatted code.

[duesee]

Hey, I did the following https://github.com/duesee/tokio-imap/commit/0e0323defa547bd91c798b55fa6e41cc1f5d80ff. This looks like a horrible commit (103 additions and 12 deletions), but IMHO makes actually sense.

The following commit https://github.com/duesee/tokio-imap/commit/39afe458e4e50d5923e637c34954d7fc1858d372 was just a "cargo fmt". What this shows is, that a huge amount of code was not formatted due to the rustfmt skip in the top-level.

Pro:

• Long-term goal is to transition to nom 5 function-style macros. Deleting the ugly attributes is even more fun when doing the transition!
• Attributes are IMHO easy to ignore and not a huge increase of cognitive load.
• Less trouble with bad formatting in those files which previously had the rustfmt skip.
• Moving parser to different file w/o rustfmt skip in top-level will not cause trouble anymore.

Contra:

• More code.
• Repetitive.
• Seems like a stupid solution.

What do you think?

[duesee]

If you like this, I will not remove the top-level ignore until the second commit. Then both will pass CI.

[djc]

I don't really like the repetitive attributes, no. I was wondering if there was a way to "invert" the skip directive on an inner scope so we could re-enable rustfmt for all those tests modules, but I couldn't find anything like that.

For now though, I've cherry-picked your second commit (which only caused a single minor merge conflict) and rebased it straight onto master, so at least the current code is clean for now. For the rest, I'd rather wait until we migrate to function parsers.

[duesee]

Maybe something like

#[rustfmt::skip]
mod skip {
    // imports not formatted :-(

    // parsers here
}

pub use skip::*;

// tests

could work. But this increases cognitive load and the indention level. Also the imports are still not covered. The tests could also be moved to a separate module. I didn't like anything of that.

I am not a fan of the repetitive attributes either, but think they are easy to understand and worth it. Is there anything you specifically dislike? The repetitiveness might be required when moving something anyway. Maybe it is possible to shorten every attribute at least? (I think #[rustfmt::skip] should work with 2018 Edition.)

[djc]

In the trade-off of how much I value having this test code and imports formatted vs the repetitive uglyness of having attributes for rustfmt on every macro, the former doesn't win out for me. In a review, conspicuously bad formatting in newly changed code will stand out for me, anyway.