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

Feature request: Option to have one "use" statement per file #5360

Open
CrendKing opened this issue Jun 1, 2022 · 9 comments
Open

Feature request: Option to have one "use" statement per file #5360

CrendKing opened this issue Jun 1, 2022 · 9 comments

Comments

@CrendKing
Copy link

CrendKing commented Jun 1, 2022

Currently imports_granularity = 'One' merges all imports into one "use" statement. In my opinion, functionally it does two separate things: 1) Combine all "use" statements; 2) apply imports_granularity = 'Crate' inside the braces. Obviously it has the advantage of having no duplicate "use". However, I also really like the style of imports_granularity = 'Module'. It's visually much clearer, and easier to modify one import without affecting other imports. Ideally, I'd love to see a style where it can combine the "one" use statement with the module style. For example,

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

I think imports_granularity is orthogonal to this one "use" statement style, so why not introduce a separate option like one_use_statement? With this new option, we will have 6 combinations in total:

  1. imports_granularity = 'Crate' + one_use_statement = false

  2. imports_granularity = 'Module' + one_use_statement = false

  3. imports_granularity = 'Item' + one_use_statement = false
    These 3 will be exactly the same as current

  4. imports_granularity = 'Crate' + one_use_statement = true
    This is basically identical to the current imports_granularity = 'One'

  5. imports_granularity = 'Module' + one_use_statement = true
    The example is shown above

  6. imports_granularity = 'Item' + one_use_statement = true

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

With this new option, imports_granularity = 'One' is no longer needed.

@calebcartwright
Copy link
Member

calebcartwright commented Jun 2, 2022

Acknowledge having seen this, but still processing (and debating with myself a bit tbh 😆)

@CrendKing
Copy link
Author

Take your time. Let me know if you need more info.

@calebcartwright
Copy link
Member

Before responding to the substance of your request, I was curious if you could elaborate on this 👇

Ideally, I'd love to see a style where it can combine the "one" use statement with the module style. For example,

Specifically, I'd like to know if you think there are genuine benefits to this or if it's more of a subjective feel/preference. Just my admittedly subjective .02, but I feel like the only impact of this is an extra level of indentation and greater risk of comments causing problems.

@CrendKing
Copy link
Author

Combining "module" with one "use" is my request. I really don't care much about "Item" + one "use", but I put there still for completeness.

As to benefit, "module" + one "use" objectively saves many redundant "use" at beginning of each line, which are replaced by indentation. However, editors are known to auto-insert indentations, but they don't auto-insert "use". Therefore, this will save a lot of typing.

Subjectively, I find one "use" more clean. Removing the duplicate "use" makes the section less cluttered. Also, the import section is usually short enough and almost always placed at top of the file, that user should not confuse this brace enclosed block with other kind of blocks below.

I'm not sure about your words around "comments causing problems". Are you saying comments are not allowed in the "use" block? I just tested it works fine.

@calebcartwright
Copy link
Member

Please remember that individual users typically approach such things from the comparatively narrower context in which they have/would utilize a feature or write code, whereas we have to account for the comparatively significantly broader context of the entirety of circumstances for any and all developers.

e.g.

Also, the import section is usually short enough and almost always placed at top of the file, that user should not confuse this brace enclosed block with other kind of blocks below.

this may be true for you, but I can assure you there are plenty of scenarios where this doesn't hold true.

Additionally 👇

Combining "module" with one "use" is my request. I really don't care much about "Item" + one "use", but I put there still for completeness.

I read the description, I understand the "what" behind the specific request you have 😉 However, similar to above, your proposed approach would actually require us to have implementation and support for all combinations, regardless of whether you'd ever personally use any of them.

I'm not sure about your words around "comments causing problems". Are you saying comments are not allowed in the "use" block? I just tested it works fine.

Trying to reorder, regroup, and/or modify the level of granularity of imports when there are comments is a challenge, and one we don't think is practically solvable particularly when there are comments within a tree (see #5311 and discussion in #4911 for more context). The additional flexibility you are requesting here would open the door for more such scenarios with problematic comments, and where rustfmt is sometimes forced to skip adjusting granularities.

@calebcartwright
Copy link
Member

As to the substance of your request and rationalization, I do agree the distinction you're drawing is a reasonable and logically defensible one. However, I view the current state as an intentional reflection of what an overwhelming number of users have expressed that they want relative to imports.

While I'm open to considering some alternative solutions to potentially support your desired formatting output, I'm highly disinclined to do so via the yet another import-related config option route you originally proposed.

There's a number of reasons for this, one prominent one being that the existing configuration surface for imports is already very large and very complex, with many interacting options and behaviors. I believe this already adds enough of a cognitive complexity burden on users and I can't really justify vectoring it out against another option. The proposed additional option would also require an implementation modification for imports_granularity as a whole, and would entirely reset the clock on stabilizing that option which I think would be discouraging for many users. Furthermore, we'd have to implement support for several formatting styles that haven't actually been requested and may never actually be used.

What I might be willing to consider is a new variant for imports_granularity that's something akin to OneModule (or anything better named) that would provide your requested output. However, I will also add that this isn't something that the rustfmt team would ever work on implementing ourselves, so in order for it to ever potentially come to fruition, you'd need to try to implement yourself or hope someone from the community does.

@CrendKing
Copy link
Author

comparatively narrower context in which they have/would utilize a feature or write code

I understand. I can't represent everyone using rustfmt. I can only share my opinion. You guys have much more experience so feel free to reject this request if you deem inappropriate.

require us to have implementation and support for all combinations

When I first wrote the request, I thought the two options doing two separate things. "one use" firstly converts individual statements into one block, and granularity does what it's doing atm, sans "One". So I was imagining one extra function, not individually implementing those combinations. However, if it's not the case, I can revise the request to just "Module" + one "use".

comments.

I read the links. From what I understand, there could be problems when changing the style from the flat to the tree style, and someone has PR to disable the normalization. Does this request introduce new scenario? I thought this is basically the same as the tree style. So disable the normalization when converting between?

@CrendKing
Copy link
Author

Just read your last comment. Totally understandable. I appreciate your hard work. Feel free to do whatever you consider appropriate. At the end of the day, it's just a different code style. I can live with it.

@calebcartwright
Copy link
Member

Just read your last comment. Totally understandable. I appreciate your hard work. Feel free to do whatever you consider appropriate. At the end of the day, it's just a different code style. I can live with it.

Sounds good. Will leave this open for a while in case you or someone else decides you'd like to pick it up and try implementing

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

No branches or pull requests

3 participants