rustfmt icon indicating copy to clipboard operation
rustfmt copied to clipboard

feat: add ignore_missing_submod option

Open tanzaku opened this issue 3 years ago • 9 comments

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.

tanzaku avatar Nov 21 '22 17:11 tanzaku

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!

ytmimi avatar Nov 21 '22 17:11 ytmimi

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.

ytmimi avatar Nov 26 '22 18:11 ytmimi

Thank you for your review. All fixed.

tanzaku avatar Nov 27 '22 15:11 tanzaku

@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.

ytmimi avatar Jan 28 '24 23:01 ytmimi

Yes, I am still interested in working on it.

tanzaku avatar Jan 29 '24 10:01 tanzaku

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 avatar Jan 29 '24 13:01 ytmimi

@ytmimi Sorry, I forgot to post earlier. I've rebased and resolved the conflicts.

tanzaku avatar Jan 30 '24 13:01 tanzaku

Thank you, I'll give this one another review later today!

ytmimi avatar Jan 30 '24 18:01 ytmimi

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!

ytmimi avatar Feb 07 '24 02:02 ytmimi