Refactor run family of functions #897
Conversation
This also unifies `write_all_files` and `write_file` functions
| pub fn run(input: Input, config: &Config) { | ||
| let (file_map, report) = format_input(input, config); | ||
|
|
||
| let ignore_errors = config.write_mode == WriteMode::Plain; |
There was a problem hiding this comment.
This roughly matches the previous behavior, but it feels ugly.
The behavior was
- swallow errors when reading from stdin (presumably because the errors and the output are written to stdout)
- print errors to stdout even with
--write-mode plainwhich kinda contradicts the reasoning behind the previous bullet
Now we swallow errors every time we print to stdout.
Note that both versions have the following bug: with --write-mode checkstyle errors are printed to stdout alongside the checkstyle xml.
Maybe we should simply print errors to stderr? Parsing errors from libsyntax go to the stderr anyway.
There was a problem hiding this comment.
Yeah, we should definitely write errors to stderr instead of stdout
| @@ -1,2 +1,2 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <checkstyle version="4.3"><file name="tests/source/fn-single-line.rs"><error line="1" severity="warning" message="Should be `fn foo_expr() { 1 }`" /><error line="1" severity="warning" message="Should be `fn foo_stmt() { foo(); }`" /><error line="1" severity="warning" message="Should be `fn foo_decl_local() { let z = 5; }`" /><error line="1" severity="warning" message="Should be `fn foo_decl_item(x: &mut i32) { x = 3; }`" /><error line="1" severity="warning" message="Should be `fn empty() {}`" /><error line="1" severity="warning" message="Should be `fn foo_return() -> String { "yay" }`" /><error line="1" severity="warning" message="Should be `fn foo_where() -> T`" /><error line="1" severity="warning" message="Should be ` where T: Sync`" /><error line="1" severity="warning" message="Should be `{`" /><error line="50" severity="warning" message="Should be `fn lots_of_space() { 1 }`" /><error line="57" severity="warning" message="Should be ` fn dummy(&self) {}`" /><error line="57" severity="warning" message="Should be `trait CoolerTypes {`" /><error line="57" severity="warning" message="Should be ` fn dummy(&self) {}`" /><error line="57" severity="warning" message="Should be `fn Foo<T>() where T: Bar {}`" /><error line="57" severity="warning" message="Should be ``" /></file></checkstyle> | |||
| <checkstyle version="4.3"><file name="tests/source/fn-single-line.rs"><error line="1" severity="warning" message="Should be `fn foo_expr() { 1 }`" /><error line="1" severity="warning" message="Should be `fn foo_stmt() { foo(); }`" /><error line="1" severity="warning" message="Should be `fn foo_decl_local() { let z = 5; }`" /><error line="1" severity="warning" message="Should be `fn foo_decl_item(x: &mut i32) { x = 3; }`" /><error line="1" severity="warning" message="Should be `fn empty() {}`" /><error line="1" severity="warning" message="Should be `fn foo_return() -> String { "yay" }`" /><error line="1" severity="warning" message="Should be `fn foo_where() -> T`" /><error line="1" severity="warning" message="Should be ` where T: Sync`" /><error line="1" severity="warning" message="Should be `{`" /><error line="50" severity="warning" message="Should be `fn lots_of_space() { 1 }`" /><error line="57" severity="warning" message="Should be ` fn dummy(&self) {}`" /><error line="57" severity="warning" message="Should be `trait CoolerTypes {`" /><error line="57" severity="warning" message="Should be ` fn dummy(&self) {}`" /><error line="57" severity="warning" message="Should be `fn Foo<T>() where T: Bar {}`" /></file></checkstyle> | |||
There was a problem hiding this comment.
<error line="57" severity="warning" message="Should be ``" /> bit at the very end is removed.
I don't know if this introduces or fixes a bug in a test case :)
|
Is there any reason to special case this |
AFAIK, there is no reason to special case and we can probably use the comment syntax. |
| @@ -403,7 +403,7 @@ pub fn format_string(input: String, config: &Config) -> FileMap { | |||
| file_map | |||
| } | |||
|
|
|||
| pub fn format(file: &Path, config: &Config) -> FileMap { | |||
| fn format(file: &Path, config: &Config) -> FileMap { | |||
There was a problem hiding this comment.
Could you rename format to format_file please? "format" seems too generic for a function which isn't API
|
Thanks! r+ with the two renamings. |
|
Renamed! FYI, this was the first automated refactoring performed with the help of IntelliJ Rust (here is the implementation) :) |
|
Thanks for the changes, and nice tool! |
Hi! This is a bit of refactoring to ease the implementation of #150.
The intent is to reduce the duplication between
run,run_from_stdinand, ideally,run_rustfmtfrom the test crate.