Add `ImportGranularity::ModuleCondensed`

[smoelius]

trafficstars

Given that imports_granularity is unstable, is this something you would consider merging?

From the docs I added:

ModuleCondensed:

Like Module, but singleton imports are included in parent modules' use statements where it doesn't introduce nested braces.

use foo::b::{f, g};
use foo::{a, b, c, d::e};
use qux::{h, i};

I have long perceived this to be Clippy's convention. I ran this on Clippy's source code and about 2/3 of the files were left unchanged. Since this supposed convention has been enforced by hand, I consider this confirmation.

[smoelius]

@flip1995 Sorry to ping you, but would you be able to comment on whether this is Clippy's preferred convention?

[flip1995]

We don't really have a import convention in Clippy, other than that we don't allow wildcard imports, enforced by one of our pedantic lints.

That being said, I like this convention and would adapt it for Clippy. Especially when 2/3 of Clippy follow it already.

[calebcartwright]

is this something you would consider merging?

In principle yes. I don't envision us adding any variants ourselves but we're open to supporting others so long as someone submits the PR like this and it doesn't add any undue maintenance burden for us. We're unlikely to be able to spend any review cycles on this for the foreseeable future though.

Haven't looked at the specifics of the proposed change yet, but would suggest including a #[unstable_variant] attr on the new config variant if you haven't already done so. Stabilization of imports granularity is still a few releases out, but it's definitely on the near term horizon and probably better to go ahead and get that attr on the new variant so that we don't accidentally forget

[smoelius1]

@calebcartwright Thank you very much for your reply.

I am currently travelling, but will add the unstable_variant attribute no later than next week.

[smoelius]

Haven't looked at the specifics of the proposed change yet, but would suggest including a #[unstable_variant] attr on the new config variant if you haven't already done so.

Done. Please let me know what else is needed. Thanks.

[calebcartwright]

@fee1-dead @CosmicHorrorDev - I know you're both busy (with both Rust and life things :smile:), but just tagging this as one for you to potentially review in case you're interested and get around to it before Yacin and/or myself.

I'm in favor of supporting the style associated with the variant, but haven't had a chance to look at the associated implementation

[smoelius]

In addition, could you add unit test cases to test this behavior? thanks.

Done. (Apologies, as I hadn't noticed the existing unit tests.)

[smoelius]

Sorry for the force push (minor grammatical adjustment). It should be good now.

[smoelius]

We're unlikely to be able to spend any review cycles on this for the foreseeable future though.

Point very well taken, but I thought I would check in. Any chance of merging this?

[smoelius]

I thought I would check in again. Any chance of merging this?

[smoelius]

Also, reorder_imports=true is on by default. can we add test cases where we set reorder_imports=false. I'd want to see what the output looks like in that case.

I added such a test case. I also took the liberty of adding such a test case for imports_granularity: Module. At least for the given input, the results are the same for both imports_granularity: Module and imports_granularity: ModuleCondensed.

Apologies, as I did not realize there was a reorder_imports option and so did not consider how it would interact with imports_granularity. I will give it some thought.

[smoelius]

@ytmimi I think imports_granularity: ModuleCondensed now works sensibly when reorder_imports is off.

Also, it appears that imports_granularity has an effect only when either reorder_imports is on, or group_imports is not Preserve (the default). So I updated the reorder_imports: false tests to use group_imports: One.

[smoelius]

@ytmimi Would it be possible to give this PR another look?