rustfmt
rustfmt copied to clipboard
feat: add ignore_missing_submod option
Resolve #5609
Made the changes suggested by ytmimi in issue #5609.
This is my first contribution, so I may have made some mistakes. I would be happy to fix them if you point them out.
Thanks for your first contribution to rustfmt 🎉. Always great to see new contributors.
I won't have a chance to review this today, but I'll be sure to give feedback once I take a look!
Can we also add the following tests to tests/rustfmt/main.rs
#[test]
fn ignore_missing_sub_mod_false() {
// Ensure that missing submodules cause module not found errors when trying to
// resolve submodules with `skip_children=false` and `ignore_missing_submod=false`
let args = [
"--config=skip_children=false,ignore_missing_submod=false",
"tests/source/issue-5609.rs"
];
let (_stdout, stderr) = rustfmt(&args);
// Module resolution fails because we're unable to find `missing_submod.rs`
assert!(stderr.contains("missing_submod.rs does not exist"))
}
#[test]
fn ignore_missing_sub_mod_true() {
// Ensure that missing submodules don't cause module not found errors when trying to
// resolve submodules with `skip_children=false` and `ignore_missing_submod=true`.
let args = [
"--emit=stdout",
"--config=skip_children=false,ignore_missing_submod=true",
"tests/source/issue-5609.rs"
];
let (stdout, _stderr) = rustfmt(&args);
let target = read_to_string(PathBuf::from("tests/target/issue-5609.rs")).unwrap();
assert!(stdout.ends_with(&target));
}
The more important of the two is ignore_missing_sub_mod_false to explicitly test the behavior when the new ignore_missing_sub option is set to false, but having the ignore_missing_sub_mod_true case right next to it serves as an inline contrast that mimics the system test that you added in tests/target/issue-5609.rs.
Thank you for your review. All fixed.
@tanzaku I know this one has been left on the backlog for a little while. I wanted to check back in and see if you were still interested in working on it.
Yes, I am still interested in working on it.
Awesome! I wanted to confirm that before jumping back in. When you have a chance, can you rebase on the latest master and fix any merge conflicts.
@ytmimi Sorry, I forgot to post earlier. I've rebased and resolved the conflicts.
Thank you, I'll give this one another review later today!
Thanks for pushing those changes. Just wanted to follow up and say that I'm planning to look at this one again later this week!