-
Notifications
You must be signed in to change notification settings - Fork 898
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
[unstable option] merge_imports #3362
Comments
Would it be possible to update this Tracking issue with more information? For example, why is this unstable? Should this always be opt-in, or is the plan to enable it by default, what remains to be done to make this option stable, etc. Which PR implemented this (e.g. #2603) ? There is a closed fmt-rfc: rust-lang/style-team#24 where @nrc said:
But that's about it, the merged RFC does not talk about it, so I'd guess there is no merged RFC saying anything about this. Is the next step towards stabilization to merge an RFC about this ? (independently of which direction that RFC goes). |
Assuming the rust compiler is behaving as intended, this can break imports. Currently, these are not semantically equivalent, but rustfmt with use alloc::vec; // imports *both* the macro and the module
use alloc::vec::Vec; use alloc::vec::{self, Vec}; // only imports the module and `Vec`. `vec!` is left out. |
Perhaps use alloc::vec::{self, Vec}; should always be converted to use alloc::{vec, vec::Vec}; |
I wish there was a third option: unmerge imports into one import per line. |
I would love |
With #3753 merged, is the only blocker renaming the option so it is a list of 3 values (Never merge, always merge, leave imports untouched)? |
With "never merge" I suppose you mean something that splits imports, or unmerge them? I suppose we want one value that turns // this
use std::{ptr, mem};
// into
use std::ptr;
use std::mem; And one value that does the opposite. And of course the third one which disables this feature altogether, not touching use statements. |
I would like to see One observation is that it is a little inconsistent about when it uses one line vs multiple lines (though I guess this is a problem without the option, but you notice it more with the option). Using E.g.,
IMO both of those should be multi-line. |
Shouldn't it just be controlled by the max line width and small_heuristics like code expressions are? |
What is missing for this to be stabilized? |
Hi! I'm asking the question again. What is missing exactly? I'm willing to help and make a PR if necessary. Should I try to change the behavior pointed by @nrc ? |
The process for stabilizing a configuration option is detailed here. That documentation also enumerates the conditions/requirements to stabilize:
As it relates to There are several open issues related to the option in addition to some of the items discussed in this thread. There are also outstanding questions regarding the default value, or even if this option should be an enum like some other config options instead of the current boolean. |
According to the stability of the implementation: Also I think it would be nice if the default was Maybe an edition change could be used to change the default formatting rules? |
Thank you @calebcartwright! I had a look. I already started to check code. @Robbepop I saw several comments in this thread about an alternative "shallow merge behavior": never import multiple modules on one line. Essentially only struct, enum, etc are merged (e.g. Regarding the option. I believe enum-like would be a good idea.
Big question. What about imports_layout? Looks like this option is similar:
|
Is there any way to do the automatic unmerging with the current rustfmt version? <rant>
I mean what am I even looking at? Import lines should be the most straightforward piece of code. Compare the above with
Wow. Boring, I know. But suddenly, if there are 2 commits adding a Surely I am missing something... |
I'd really like to see this stabilized (preferably with options "Never" and "Shallow"). |
I think another option that I’ve seen in some codebases is the use of only one use {
futures::{compat::Compat01As03, future::BoxFuture, prelude::*},
std::{
fmt::{self, Display},
result,
},
}; rather than use futures::{compat::Compat01As03, future::BoxFuture, prelude::*};
use std::{
fmt::{self, Display},
result,
}; |
I only realized the question was about naming after a second reading, I think it would be helpful to put more emphasis on it in the original question. Also, alternate naming suggestion for |
This comment has been minimized.
This comment has been minimized.
I would like to propose We have several options for changing the formatting of imports in some way:
I think Why I found the following terminology being used within existing rustfmt options:
(2) suggests that our new option controls how imports are organized into blocks:
This looks fairly consistent with one exception: I believe syntactially, a block is identified by curly braces |
I understand that thinking, and have gone through a few iterations of it myself. However, I think the usage of Though the I think an additional modifier in the config option name could be helpful, but I'm opposed to |
I agree with that, Edit: Based on the number of upvotes, people also seem to be happy with the |
imports_layout = "Horizontal"
imports_granularity = "Module" |
Agreed. I like this one a lot more. Given the work in flight, we'll first need to finalize the config option name and the name of the variants that map to the current true/false values for |
Alternative suggestions for |
I think the verb "grouping" best describes what this option does. Something like |
Obviously a variant names applicability is dependent upon the option itself, but with the main option names discussed so far |
Understand the thinking with this one, but it's another case where it'd be problematic in the broader context since there's also a desire (and a new, unstable config option group_imports) to support moving/regrouping use items separately from the granularity (refs #4107) |
Sorry to hear that there are problems with |
Alternate suggestion for |
Further notes regarding consistency:
|
Understand the thrust of the point, but this does need a minor correction. It's true that there are no option variants that utilize https://github.com/rust-lang/rustfmt/blob/rustfmt-1.4.31/Configurations.md#version |
Thanks to everyone who weighed in and voted! At this point I feel we have more than sufficient consensus to move forward with the first portion of this (#4600) to make the new configuration option and a couple of the variants available for consumption. The new config option will be named imports_granularity, and will initially include the Crate, Module, and Preserve variants. After incorporating the new option into the codebase I'll open up new individual issues to track the implementation of the respective additional variants and their associated styles, and then at some point down the road we'll start a new thread to track the stabilization of Happy to let the discussion continue here on the names of the other variants (e.g. |
Since |
It's not really about whether or not there's contestation. The actual implementation already exists for the ones I mentioned above (rustfmt is capable of doing the corresponding formatting), but there is no such implementation for the others. If anyone is interested in doing the work to actually implement support for We're proceeding with what's implemented and possible today, and the others will be added at some point down the road. We just want to go ahead and start letting folks use what's already available now, as opposed to making everyone wait until everything is done to be able to use any variant. |
Thanks again everyone! I'm going to go ahead and close this issue as it's served it's purpose and is no longer necessary. To summarize:
|
Summary: To make the codebase a bit more consistent and reduce conflicts. Similar to Java. Note: The fbsource Rust lint does not respect this file so it cannot conflict with the fbcode rustfmt config. That means a more preserving (for what to group in `{}` and what not), less controversial (when to use `{}` grouping is controversial) choice `imports_layout = "HorizontalVertical"` (like [black](https://github.com/psf/black) in Python) cannot be used at present. See also: - rust-lang/rustfmt#3361 - rust-lang/rustfmt#3362 - https://fb.workplace.com/groups/rust.language/posts/6720351014680123/ - https://fb.workplace.com/groups/rust.language/posts/5429720200409884/ This adds the config without changing the files. Reviewed By: yancouto Differential Revision: D31746731 fbshipit-source-id: 8e171829fd53691a59bf3b80cdc500c0b3993ba5
Summary: Pull Request resolved: #1 Seeing a lot of warnings when running `cargo fmt`, ``` Warning: the `merge_imports` option is deprecated. Use `imports_granularity="Crate"` instead ``` [GitHub issue on the deprecation](rust-lang/rustfmt#3362 (comment)) As we already have the imports_granularity="Item", it seems we can get rid of the deprecated option? Reviewed By: ndmitchell Differential Revision: D37313506 fbshipit-source-id: ce2da68de2dac15df5df3e6803be67baaa42cb15
Part of #155 This is going to be a fun one 😆 I've pushed two preview commits: the first one imports using the `Crate` granularity, the other one with `Module` granularity. Disclaimer: I'm biased 😃 I've used this style since I started using Rust and it's also being used by vast majority of projects in the ecosystem that I've used or looked at; it's also used by the Rust compiler, Cargo and the tools like Rustfmt or Clippy. Here's a summary of the possible options: rust-lang/rustfmt#3362. Here is an RFC discussion about setting on a variant: rust-lang/style-team#140, I highly recommend reading it. To give a brief summary, it seems that people stand behind two options: - `Module` - good trade-off between readability and conciseness - `Item` - minimizes conflicts when many people edit the same files often To bring back some of the arguments raised in favor of the `Module` and explain some of my reasoning as well: - it naturally settles on the module as the, well, module/boundary from which items can be imported... - thus, helps build the intuition how to use and structure Rust crates - less verbose than Java-style (`Item`) imports, which can often explode and take unreasonable amount of space (and we don't really benefit enough from minimized conflicts as we probably won't be a team of 50 or the like)... - but repeats enough information to quickly trace a module path rather than having to reconstruct the paths from the `crate`-style use import tree... - and already often takes less space in our case, line-wise; - quite good `grep`pability
Tracking issue for unstable option: merge_imports
The text was updated successfully, but these errors were encountered: