Skip to content

config: Rename Replace WriteMode variant to Backup #888

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

Conversation

kamalmarhubi
Copy link
Contributor

The new variant name more closely matches its purpose. The rustfmt
binary is modified to accept --write-mode replace as a synonym for the
new name.

The new variant name more closely matches its purpose. The `rustfmt`
binary is modified to accept `--write-mode replace` as a synonym for the
new name.
@kamalmarhubi
Copy link
Contributor Author

r? @marcusklaas

@kamalmarhubi
Copy link
Contributor Author

NB the change in rustfmt binary does not pick up an explicit write-mode in the rustfmt.toml file. Projects with write_mode = "replace" in their project file will see a decode failure. I think this is acceptable, as it's not really a setting that belongs in the project file anyway (cf #871). If you prefer, I can implement rustc_serialize::Decodable directly or mess with the macros to make it work.

@marcusklaas
Copy link
Contributor

I don't feel confident enough about this to merge it. What do you think, @nrc?

@nrc
Copy link
Member

nrc commented Mar 31, 2016

I'm not really sure about this change, since backup is a side effect of the main action (which is overwriting). I guess the ideal would be OverwriteWithBackup, but that is unpleasantly verbose.

While I agree that the replace/overwrite distinction is vague and annoying, I don't think it is worth changing unless we have a really superior alternative.

@nrc nrc closed this Mar 31, 2016
@kamalmarhubi kamalmarhubi deleted the writemode-replace-to-backup branch April 1, 2016 20:40
@kamalmarhubi kamalmarhubi restored the writemode-replace-to-backup branch April 1, 2016 20:40
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