feat: add ignore_missing_submod option #5611

tanzaku · 2022-11-21T17:45:53Z

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.

ytmimi · 2022-11-21T17:52:46Z

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 · 2022-11-26T17:55:55Z

tests/source/issue-5609.rs

-// rustfmt-config: issue-5609.toml
+// rustfmt-ignore_missing_submod: true

 mod missing_submod;



The source file actually isn't necessary either. Since we're not doing any formatting to the file the target file is enough for this test.

Actually, on second thought, I think keeping the source test is good to demonstrate that formatting is still happening. Can we explicitly set skip_children=false. skip_children=false is the current default, and when this is set we'll try to resolve sub modules. Again, this is the current default, but formatting would work just as well if skip_children=true, since we then wouldn't try to recursively resolve children, and therefore the ignore_missing_submod option wouldn't have any affect.

In case we change the default behavior of skip_children to be true in the future explicitly setting false will ensure this test keeps working as intended.

ytmimi · 2022-11-26T17:59:18Z

src/config/mod.rs

@@ -173,6 +173,7 @@ create_config! {
         or they are left with trailing whitespaces";
    ignore: IgnoreList, IgnoreList::default(), false,
        "Skip formatting the specified files and directories";
+    ignore_missing_submod: bool, false, true, "Ignore missing submodule error";


All new options are unstable by default. Can you please change this to:

Suggested change

ignore_missing_submod: bool, false, true, "Ignore missing submodule error";

ignore_missing_submod: bool, false, false, "Ignore missing submodule error";

ytmimi · 2022-11-26T18:56:27Z

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 · 2022-11-26T19:06:42Z

Configurations.md

@@ -1288,6 +1288,14 @@ If you want to ignore every file under the directory where you put your rustfmt.
 ignore = ["/"]
 ```

+## `ignore_missing_submod`
+
+Ignore missing submodule error.


I think we need to elaborate on this description. I think we should explain that normally missing modules will lead to module not found errors and rustfmt won't format anything.

Fixed. Most of the messages you wrote are used, but I added "By default" at the beginning of the sentence to clarify the behavior by default.

ytmimi · 2022-11-26T19:08:04Z

Configurations.md

+
+- **Default value**: `false`
+- **Possible values**: `true`, `false`
+- **Stable**: Yes


All new options are unstable by default, so we'll need to update this. We don't need to create a tracking issue for the option just yet, but we'll need to mark this as unstable.

tanzaku · 2022-11-27T15:01:51Z

Thank you for your review. All fixed.

ytmimi · 2024-01-28T23:39:44Z

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

tanzaku · 2024-01-29T10:41:15Z

Yes, I am still interested in working on it.

ytmimi · 2024-01-29T13:40:12Z

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.

tanzaku · 2024-01-30T13:13:59Z

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

ytmimi · 2024-01-30T18:25:31Z

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

ytmimi

First, I wanted to say thank you for your patience. I know this was left on the backlog for quite a while, and I'm glad that you're still interested in helping out with this one.

Overall, I think the implementation that you've put together would work if we went with a boolean option, however the team has had some discussions about shifting our error reporting towards an Error, Warn, and Ignore model. Given that we're likely heading in this direction for our other error reporting I'd really like to apply those variants to this new configuration option.

ytmimi · 2024-01-31T05:01:38Z

Configurations.md

+- **Default value**: `false`
+- **Possible values**: `true`, `false`


On thing the team has discussed is moving away from a binary error model. I think if we land this, I'd like to change from true and false to something like Warn, Error, Ignore. With that in mind I think it a more appropriate name would be something like report_missing_submod. Let me know if you can come up with anything better!

Update Configuration.md on this commit. 4bb0453

ytmimi · 2024-01-31T05:05:49Z

src/modules.rs

+                .or_else(|err| match err.kind {
+                    ModuleResolutionErrorKind::NotFound { file: _ }
+                        if self.ignore_missing_submod =>
+                    {
+                        Ok(None)
+                    }
+                    _ => Err(err),
+                })


I'd like to rework this with Error, Warn, and Ignore variants in mind. I think the only difference is that if Warn is set we'll emit an error message, but still continue with Ok(None).

Thank you for the review! When emitting a message for a 'warn', is it sufficient to simply use eprintln! to output to stderr? Or is there a more appropriate method you could suggest?

Fixed. f678369

For now, I implemented direct output with eprintln.
(However, I think I might be better off using FormatReport.)

ytmimi · 2024-01-31T05:09:10Z

tests/rustfmt/main.rs

@@ -200,3 +200,30 @@ fn rustfmt_emits_error_when_control_brace_style_is_always_next_line() {
    let (_stdout, stderr) = rustfmt(&args);
    assert!(!stderr.contains("error[internal]: left behind trailing whitespace"))
 }
+


will likely need to rework the tests slightly to accommodate Error, Warn, and Ignore variants.

Fixed test on this commit. fcc06f9

ytmimi · 2024-02-07T02:01:12Z

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

Just had a chance to look this over again and test out these changes locally. Things are working great! Thank you again for your help here. Marking this one as ready-to-merge, but will hold off on merging this one right away to give @calebcartwright a chance to look it over.

ytmimi self-assigned this Nov 21, 2022

ytmimi added the pr-not-reviewed label Nov 21, 2022

ytmimi reviewed Nov 26, 2022

View reviewed changes

ytmimi added pr-waiting-on-author and removed pr-not-reviewed labels Nov 26, 2022

ytmimi added pr-follow-up-review-pending and removed pr-waiting-on-author labels Nov 28, 2022

ytmimi mentioned this pull request Nov 12, 2023

Allow for missing modules #5962

Closed

tanzaku added 6 commits January 30, 2024 00:45

Add ignore_missing_submod config option

b9e2897

Remove an unnecessary configuration file for testing

34fd630

Add skip_children option explicitly

b6d27d3

Mark the ignore_missing_submod option as unstable

2faae32

Add tests

aa4cd26

Update ignore_missing_submod option description

50e7197

tanzaku force-pushed the issue-5609-add-ignore_missing_submod-option branch from 03a5767 to 50e7197 Compare January 29, 2024 16:04

ytmimi requested changes Jan 31, 2024

View reviewed changes

ytmimi added pr-waiting-on-author and removed pr-follow-up-review-pending labels Jan 31, 2024

Update Configuration.md

4bb0453

tanzaku added 2 commits February 1, 2024 20:22

Changes error reporting to an Error, Warn, and Ignore model

f678369

Fix tests

fcc06f9

tanzaku requested a review from ytmimi February 2, 2024 16:45

ytmimi approved these changes Feb 15, 2024

View reviewed changes

ytmimi added pr-ready-to-merge release-notes Needs an associated changelog entry and removed pr-waiting-on-author labels Feb 15, 2024

ytmimi mentioned this pull request Feb 17, 2024

Module refactors #6062

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add ignore_missing_submod option #5611

feat: add ignore_missing_submod option #5611

tanzaku commented Nov 21, 2022

ytmimi commented Nov 21, 2022

ytmimi Nov 26, 2022

ytmimi Nov 26, 2022

ytmimi Nov 26, 2022

ytmimi commented Nov 26, 2022

ytmimi Nov 26, 2022

tanzaku Nov 27, 2022

ytmimi Nov 26, 2022

tanzaku commented Nov 27, 2022

ytmimi commented Jan 28, 2024

tanzaku commented Jan 29, 2024

ytmimi commented Jan 29, 2024

tanzaku commented Jan 30, 2024

ytmimi commented Jan 30, 2024

ytmimi left a comment

ytmimi Jan 31, 2024

tanzaku Feb 1, 2024

ytmimi Jan 31, 2024

tanzaku Feb 1, 2024

tanzaku Feb 1, 2024

ytmimi Jan 31, 2024

tanzaku Feb 1, 2024

ytmimi commented Feb 7, 2024

ytmimi left a comment •

edited

Loading

	ignore_missing_submod: bool, false, true, "Ignore missing submodule error";
	ignore_missing_submod: bool, false, false, "Ignore missing submodule error";

		- Default value: `false`
		- Possible values: `true`, `false`

feat: add ignore_missing_submod option #5611

Are you sure you want to change the base?

feat: add ignore_missing_submod option #5611

Conversation

tanzaku commented Nov 21, 2022

ytmimi commented Nov 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ytmimi commented Nov 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanzaku commented Nov 27, 2022

ytmimi commented Jan 28, 2024

tanzaku commented Jan 29, 2024

ytmimi commented Jan 29, 2024

tanzaku commented Jan 30, 2024

ytmimi commented Jan 30, 2024

ytmimi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ytmimi commented Feb 7, 2024

ytmimi left a comment • edited Loading

Choose a reason for hiding this comment

ytmimi left a comment •

edited

Loading