-
Notifications
You must be signed in to change notification settings - Fork 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
Rust rewrite check_diff (Skeleton) #6166
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
#[test] | |
fn self_tests() { | |
init_log(); | |
let mut files = get_test_files(Path::new("tests"), false); | |
let bin_directories = vec!["cargo-fmt", "git-rustfmt", "bin", "format-diff"]; | |
for dir in bin_directories { | |
let mut path = PathBuf::from("src"); | |
path.push(dir); | |
path.push("main.rs"); | |
files.push(path); | |
} | |
files.push(PathBuf::from("src/lib.rs")); | |
let (reports, count, fails) = check_files(files, &Some(PathBuf::from("rustfmt.toml"))); | |
let mut warnings = 0; | |
// Display results. | |
println!("Ran {count} self tests."); | |
assert_eq!(fails, 0, "{fails} self tests failed"); | |
for format_report in reports { | |
println!( | |
"{}", | |
FormatReportFormatterBuilder::new(&format_report).build() | |
); | |
warnings += format_report.warning_count(); | |
} | |
assert_eq!(warnings, 0, "Rustfmt's code generated {warnings} warnings"); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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