Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add flatten_imports config option (inverse of merge_imports) #4553

Closed
wants to merge 2 commits into from

Conversation

maackle
Copy link

@maackle maackle commented Nov 25, 2020

The title says it all, and Configurations.md shows it in action.

In doing a huge refactor that involved moving large chunks of code from one crate to another, I found it crucial to be able to flatten imports so that the import paths I needed to change were easily greppable, which is not the case for nested imports. Without this option, I would have had to do hundreds of manual import tweaks.

This is not thoroughly tested, especially around self imports, but worked fine for my use case, which is a large codebase with a variety of imports (https://github.com/holochain/holochain). If you think this would be a good addition, it needs a stabilization tracking issue before merging. I'd be happy to help with any loose ends, too.

@calebcartwright
Copy link
Member

Thank you for the PR @maackle. I don't know if you've had an opportunity to review the highly related RFC in rust-lang/style-team#140, but if not would strongly encourage you to do so.

The short version is that we need to move to a different, single enum-based option that allows users to control how imports are formatted, and that will support multiple strategies.

I appreciate the effort you've put into this, but adding a separate boolean based option would be taking us in the opposite direction so this won't be mergeable as-is.

If you're interested in changing the approach to better align with the strategy settled on in the RFC that would be fantastic, but no worries if not!

@maackle
Copy link
Author

maackle commented Nov 26, 2020

@calebcartwright thanks for the detailed response pointing that out. The effort wasn't great, it took me about 15 minutes to put it all together (the code is really clean so it was easy!), so I'd be happy to eventually reformat this to match the RFC. However, reading the RFC, it seems that there is a stated desire to allow multiple import formatting strategies, but no strategies other than "merge" actually exist at the moment. So, perhaps this should wait until some other strategies get decided on and gathered under an enum? If I'm reading that wrong let me know.

@calebcartwright
Copy link
Member

There's also the issue tracking stabilization of the original merge_imports option with a great summary of the strategies in #3362 (comment)

However, reading the RFC, it seems that there is a stated desire to allow multiple import formatting strategies, but no strategies other than "merge" actually exist at the moment.

Well, certainly the current behavior (which will continue to be the default) of doing nothing/preserving the original exists, and then IIRC with merge_imports = true the formatting generally aligns to the strategy described as "module" or "shallow" in those threads. What's definitely missing is what you'd originally proposed here, referred to with phrases like "item", "java", "flatten", etc.

Some of the other strategies, like crate/everything may not be strictly applicable to merge_imports as merging across groups brings related options like group_imports into the picture, but the main benefit of moving to the new approach is that we can always add additional variants and associated strategy to the enum as they come along/someone is sufficiently motivated to implement them.

So, perhaps this should wait until some other strategies get decided on and gathered under an enum? If I'm reading that wrong let me know

There's actually nothing but a lack of bandwidth preventing the first steps of this process from occurring. The first steps would be:

  • adding the new config option with the initial two variants to cover current behavior (Preserve which maps to current default merge_imports = false and whatever will map to current merge_imports = true)
  • adding soft deprecation of merge_imports to warn/notify users setting merge_imports to true and mapping that to the aforementioned variant on the new option

Then, should you or anyone else be interested in the item/flatten/java style, that variant and formatting approach could be added independently of any of the other variants/strategies. There's definitely a strong community desire for this strategy, and I think the only real outstanding question would be what to name the variant and the associated bikeshedding

@calebcartwright
Copy link
Member

Thanks again for the PR @maackle, but I think I'm going to go ahead and close this PR given the above discussion and overall planned strategy for supporting import merging/flattening

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants