Rename merge_imports to imports_granularity and add a Module option.

[goffrie]

This renames the existing true/false options to Crate/Never, then adds a new Module option which causes imports to be grouped together by their originating module.

The crate/module option terminology is taken from #3362 (comment) which makes the most sense to me. I haven't implemented the other options there, though.

[goffrie]

@calebcartwright Is this closer to what you had in mind?

Also I don't know if we want to do anything special to avoid breaking people's configuration files re. merge_imports=true, though I suppose that's why it's unstable.

[calebcartwright]

@calebcartwright Is this closer to what you had in mind?

Also I don't know if we want to do anything special to avoid breaking people's configuration files re. merge_imports=true, though I suppose that's why it's unstable.

I think the best way to avoid this will be to come up with a new config option that's enum based, so that we can present a deprecation warning to folks using merge_imports and potentially do an internal mapping of the true/false value for merge_import to the corresponding strategy on the new option (so as long as both new_config_thing and merge_imports aren't both set)

[goffrie]

Hm, I couldn't think of a suitable new name so I opted to keep it as-is, but I made true/false parse as though they were Crate/Never and print a warning. Does that seem reasonable?

[calebcartwright]

Thank you for the updates @goffrie. Please note that the CI failure on beta is not spurious, and would need to be resolved.

Hm, I couldn't think of a suitable new name so I opted to keep it as-is, but I made true/false parse as though they were Crate/Never and print a warning. Does that seem reasonable?

Please note that Never variants within other config options carry potential formatting changes in their respective formatting areas. merge_imports being false is a no-op/change nothing, which is more analogous to the Preserve (or Off in the case of Widthheuristics) variants that respect/preserve whatever the programmer wrote and make no changes . While I realize the proposed Never variant here is intended to convey that same "change-nothing" idea, I want to maintain consistency across the various config options as much as possible to avoid potential user confusion ("why does Never do this for foo but something different for bar") by having this variant be Preserve or Off

As for the name, I still think we'll need a new name that doesn't carry such a boolean connotation. We've had previous config options with very boolean sounding names (e.g. use_small_heuristics) but non-boolean values which users have reported finding confusing, and want to avoid introducing such a case.

A relatively common naming pattern for enum-based options is to have a _style suffix (see brace_style, indent_style, newline_style, etc.) so perhaps we could follow that with something like imports_merge_style?

[goffrie]

OK, that reasoning makes sense to me. I've renamed the option to imports_merge_style and the default to Preserve, but merge_imports continues to be accepted.

Sadly the code that does the forwarding is a bit messy, but I couldn't find a good way to clean it up. Let me know if you have any suggestions...

Also the beta tests should be fixed now.

[calebcartwright]

Thanks for the updates and your work on this! I know there's a bit of an initial hill we have to climb here to pivot to supporting multiple strategies, but this will be a big enabler in providing some features highly desired by the community.

Looks pretty good overall, and have left some inline comments below on a few items. Would also be good to add some additional tests that include combinations of various values of related config options (e.g. group_imports and reorder_imports)

[calebcartwright]

Is there a way we could format with the Module merge style without first converting it to the Crate style? I realize that could potentially involve some non-minimal refactoring of the existing code to split out the reusable bits from the Crate-specific strategy, but I think we'll want to go that route given the additional strategies we know we'll need to support eventually

[calebcartwright]

Alternatively if you'd prefer, I'd be okay with just adding // TODO comments inline here and deferring this to a later date. The new config option and mapping of the old ones is a good enhancement in its own right, so don't necessarily need to block

[goffrie]

Yeah, I'm not a huge fan of this code either. I'll try to rewrite it.

[johnmave126]

I am wondering whether the name imports_merge_style could be ambiguous. For example, if the option is "Module", it isn't immediately clear whether it means merging modules (one use per crate), or grouping by modules (one use per module).

[calebcartwright]

Thanks @johnmave126 - i think we're going to change that (I almost posted here earlier) but going to utilize one of the other threads instead to allow us to keep the discussion on this PR centered on the implementation

[johnmave126]

@calebcartwright Just to confirm. Is the thread for discussion rust-lang/style-team#140?

[calebcartwright]

@calebcartwright Just to confirm. Is the thread for discussion rust-dev-tools/fmt-rfcs#140?

Here: #3362 (comment)

I know it's a little tricky as there's some bifurcation in the discussion, but that existing thread in this repo felt like the best available option

[goffrie]

I cleaned the docs up a bit, and now instead of merging and unmerging in separate steps, we merge up to the desired granularity from the start, so the code is a bit cleaner now.

[goffrie]

CI failure on Windows seems to be some cargo-make issue, possibly upstream.

[calebcartwright]

CI failure on Windows seems to be some cargo-make issue, possibly upstream.

Looks like a linker error, more likely caused by some changes on the GH runner environments rather than an issue with cargo-make itself. Will take a look at that in a bit

[calebcartwright]

CI's fixed now, you can rebase or grab 9a19515 to get the windows gnu jobs passing again.

Have only taken a quick glance through the updates but they looked pretty great! Going to give it a few more days for the community to weigh in on the option and variant names, after which I imagine this will be good to (other than potentially renaming a couple things)

[calebcartwright]

Thanks again @goffrie! Based on the discussions in the linked threads, we're going to go with imports_granularity as the name of the config option. Sorry for requesting one more name change on you 😄 but this is a feature with a ton of interest from the community and one we wanted to solicit as much input as possible.

After this update I believe we'll be good to merge and hopefully get this shipped in v1.4.32

[goffrie]

Done :)

[karyon]

backported in #4634