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

New disable_all_formatting config option #1297

Merged
merged 2 commits into from
Feb 7, 2017
Merged

New disable_all_formatting config option #1297

merged 2 commits into from
Feb 7, 2017

Conversation

cmbrandenburg
Copy link
Contributor

This pull request adds a new Boolean configuration option called disable_all_formatting, which causes rustfmt to skip all formatting in all files.

This option could be useful for someone who:

  1. Has configured their text editor to automatically run rustfmt when saving a Rust source file, and,
  2. Is editing source files in a Rust project that's not set up to use rustfmt.

Currently, in that scenario, the programmer will often inadvertently format lots of Rust code—even code unrelated to what the programmer is working on. The programmer must then undo all the formatting changes while keeping their intended changes.

Whereas, with the new disable_all_formatting option, the programmer can drop this .rustfmt.toml into the project's top-level directory and avoid formatting unrelated sections of code.

disable_all_formatting: true

@brandur
Copy link

brandur commented Feb 6, 2017

At the risk of piling onto this one, I just wanted to say that I've run into this problem a few times now, and was mildly astonished that no current resolution exists.

I've enabled Rust.vim with let g:rustfmt_autosave = 1, which works fine for my own projects or any others that have a rustfmt.toml. However, when I come across a project not setup for rustfmt, there doesn't appear to be any way to avoid accidental reformat-the-world situations except for me to reconfigure Rust.vim at a global level.

The proper course of action is probably to convince the project owner to support rustfmt, but it'd be nice to have an escape hatch to temporarily avoid having to shave that yak by just creating a local rustfmt.toml with a disable directive in it. I came here thinking to submit a PR for it, but it turns out that this very timely one was already here.

Onto the color of the shed: the naming of disable_all_formatting seems to be a little redundant given that you're already tweaking an option in rustfmt.toml and we know that rustfmt's raison d'être is formatting. Something more succinct like "disable" might be enough.

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.

Thanks for the PR! Seems like a good idea to add this, although it does seem to be working around a deficiency in the editor, which is a bit of a shame. A couple of comments need to be addressed before landing, but the strategy is good.

@@ -0,0 +1,4 @@
// rustfmt-disable_all_formatting: true
Copy link
Member

Choose a reason for hiding this comment

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

This test doesn't do anything because there is not corresponding file in tests/target. However, since you only want to do an idempotent test, you could move it to tests/target and it would be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I misread the Contributing.md instructions and put the file in the wrong directory.

Creating a test is as easy as creating a new file in ./tests/source/ and an equally named one in ./tests/target/. If it is only required that rustfmt leaves a piece of code unformatted, it may suffice to only create a target file.

src/lib.rs Outdated
@@ -339,6 +339,10 @@ fn format_ast<F>(krate: &ast::Crate,
-> Result<(FileMap, bool), io::Error>
where F: FnMut(&str, &mut StringBuffer) -> Result<bool, io::Error>
{
if config.disable_all_formatting {
Copy link
Member

Choose a reason for hiding this comment

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

You probably want this check in the caller, rather than here, since there might be line-based reformating as well as AST-based reformatting

@cmbrandenburg
Copy link
Contributor Author

Does anyone else have an opinion about the config option name? @brandur recommends shortening it from disable_all_formatting to disable. Either is fine by me.

@nrc
Copy link
Member

nrc commented Feb 7, 2017

Thanks for the changes!

I don't think either name for the option is ideal, but I can't think of a better one.

@brandur
Copy link

brandur commented Feb 7, 2017

FWIW, I don't feel too strongly about it either. If no one feels too strongly one way or the other, I'd defer to the author of the patch.

@nrc nrc merged commit f2c867d into rust-lang:master Feb 7, 2017
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.

3 participants