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

config: Use write_mode from config #812

Merged
merged 1 commit into from Feb 9, 2016

Conversation

kamalmarhubi
Copy link
Contributor

This commit tidies up handling of write_mode by setting it in the
config at the start, and removing the write_mode parameter threaded
throughout the formatting process.

@@ -112,6 +113,14 @@ fn match_cli_path_or_file(config_path: Option<PathBuf>,
fn update_config(config: &mut Config, matches: &Matches) {
config.verbose = matches.opt_present("verbose");
config.skip_children = matches.opt_present("skip-children");

let write_mode = matches.opt_str("write-mode").map(|ref s| {
WriteMode::from_str(s).expect(&format!("Invalid write-mode: {}", s))
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't panic due to invalid user input. We should exit gracefully with the message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@nrc
Copy link
Member

nrc commented Feb 8, 2016

LGTM, just that one comment to address

@nrc
Copy link
Member

nrc commented Feb 9, 2016

I prefer the version you've currently implemented, thanks! However, you now have a test error.

Could you also squash the commits before I merge?

This commit tidies up handling of `write_mode` by setting it in the
config at the start, and removing the `write_mode` parameter threaded
throughout the formatting process.
@kamalmarhubi
Copy link
Contributor Author

However, you now have a test error.

The formating tool that tests its own formatting... keeps biting! :-)

Could you also squash the commits before I merge?

Should be all done!

@nrc
Copy link
Member

nrc commented Feb 9, 2016

Awesome, thanks!

nrc added a commit that referenced this pull request Feb 9, 2016
@nrc nrc merged commit 347cbb1 into rust-lang:master Feb 9, 2016
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.

None yet

2 participants