Rust rewrite check_diff (Skeleton)#6166
Conversation
ytmimi
left a comment
There was a problem hiding this comment.
Let's take a different approach here. Instead of creating a new binary target for rustfmt, lets create a check_diff crate within this repo. Similar to how config_proc_macro is its own crate.
|
Thanks for the insightful discussion on Zulip. It cleared up alot of doubts and gave a clearer picture of what is the end goal. :) |
|
@rustbot ready |
c5f2777 to
13d9cda
Compare
|
After squashing the commits and rebasing we'll be good to on this one |
After thinking about it a little more I think we need to make some changes to the CliInputs struct.
ytmimi
left a comment
There was a problem hiding this comment.
Take a look at the most recent review when you have a chance. Overall I think we're heading in the right direction, but I'd like us to make some further changes.
There was a problem hiding this comment.
Can we make sure to run rustfmt built from source on these files cargo run --bin rustfmt -- check_diff/src/main.rs I believe there are certain lines that exceed the max_width.
It would probably be worth adding the check_diff crate to rustfmt's self_test to ensure that these files stay formatted:
Lines 382 to 411 in 871113e
There was a problem hiding this comment.
Added some code here to deal with external crates instead of modifying the existing loop.
48681a4 to
efb790a
Compare
| let external_crates = vec!["check_diff"]; | ||
| for external_crate in external_crates { | ||
| let mut path = PathBuf::from(external_crate); | ||
| path.push("src"); | ||
| path.push("main.rs"); | ||
| files.push(path); | ||
| } |
There was a problem hiding this comment.
Just wanted to note that if we add a lib.rs at some point then we'll need to remember to add that here too
ytmimi
left a comment
There was a problem hiding this comment.
Thanks for working through all the changes I requested. I think we're good to go here!
Closes first task from #6170

Help screen usage
Sample usage with all fields present

Sample error when 1 compulsory field is missing

Update: Screenshots