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

Rust rewrite check_diff (Skeleton) #6166

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

benluiwj
Copy link
Contributor

@benluiwj benluiwj commented May 20, 2024

Closes first task from #6170
Help screen usage
Screenshot 2024-05-30 at 4 11 25 PM

Sample usage with all fields present
Screenshot 2024-05-30 at 4 11 45 PM

Sample error when 1 compulsory field is missing
Screenshot 2024-05-30 at 4 12 15 PM

Update: Screenshots

@benluiwj benluiwj changed the title Rust rewrite of check_diff (Skeleton) Rust rewrite check_diff (Skeleton) May 20, 2024
Copy link
Contributor

@ytmimi ytmimi left a 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.

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.lock Outdated Show resolved Hide resolved
@benluiwj
Copy link
Contributor Author

Thanks for the insightful discussion on Zulip. It cleared up alot of doubts and gave a clearer picture of what is the end goal. :)

@benluiwj benluiwj requested a review from ytmimi May 24, 2024 03:07
@benluiwj
Copy link
Contributor Author

@rustbot ready

@rustbot
Copy link
Collaborator

rustbot commented May 24, 2024

Error: The feature shortcut is not enabled in this repository.
To enable it add its section in the triagebot.toml in the root of the repository.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@ytmimi
Copy link
Contributor

ytmimi commented May 30, 2024

After squashing the commits and rebasing we'll be good to on this one

ytmimi
ytmimi previously approved these changes May 30, 2024
check_diff/src/main.rs Outdated Show resolved Hide resolved
@ytmimi ytmimi dismissed their stale review May 30, 2024 02:20

After thinking about it a little more I think we need to make some changes to the CliInputs struct.

@benluiwj benluiwj requested a review from ytmimi May 30, 2024 08:17
Copy link
Contributor

@ytmimi ytmimi left a 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.

check_diff/src/main.rs Outdated Show resolved Hide resolved
Copy link
Contributor

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:

rustfmt/src/test/mod.rs

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");
}

Copy link
Contributor Author

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.

@benluiwj benluiwj requested a review from ytmimi June 1, 2024 05:34
Comment on lines +394 to +400
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);
}
Copy link
Contributor

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

Copy link
Contributor

@ytmimi ytmimi left a 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!

@ytmimi ytmimi merged commit 76ef162 into rust-lang:master Jun 4, 2024
27 checks passed
@benluiwj benluiwj deleted the rust-checkdiff-skeleton branch June 4, 2024 02:55
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

3 participants