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

order stacks of imports and/or pub mod delcarations #298

Closed
softprops opened this issue Sep 10, 2015 · 21 comments
Closed

order stacks of imports and/or pub mod delcarations #298

softprops opened this issue Sep 10, 2015 · 21 comments

Comments

@softprops
Copy link

I'm looking to apply rustfmt to a couple projects. So far my experience is good except for one surprise when I opt to order my imports it seems like the ordering only applies to a single line of imports, those within brackets. It would be nice to also have this order stacks of use declarations and for bonus points pub mod declarations. It this is not doable let me know. If it easy doable and you can point me in the right direction, also let me know. I'd love to contribute something like this.

@nrc
Copy link
Member

nrc commented Sep 10, 2015

I think this is both doable and desirable, however it is not super-easy, but you could certainly have a go. You should look at how we format import items already, specifically look at src/imports.rs and visit_item in src/visitor.rs.

When we walk the AST, we visit one item at a time. In order to format all imports at once, we would need to collect all the import nodes (presumably in a separate pass over the AST) and format them at once. Note that we only want to do this for a given scope, if we were to look at all imports from a walk, we'd get every import in every module. This means we probably need to do a different walk over items in visit_mod (which we don't currently override, see https://dxr.mozilla.org/rust/source/src/libsyntax/visit.rs#61).

If you're keen and have questions once you've looked over that code, feel free to ping me on irc with questions.

@nrc
Copy link
Member

nrc commented Sep 10, 2015

Oh, and I would start with just imports and do mod declarations as a followup, they will be similar, but different and will want separate options.

It would be really cool to see this done!

@nrc nrc added the fun! :) label Sep 10, 2015
@softprops
Copy link
Author

cool. like the fun! label :)

@nrc
Copy link
Member

nrc commented Sep 10, 2015

(fun because (I think) it will be the first bit of non-local reformatting)

@jdm
Copy link
Contributor

jdm commented Sep 11, 2015

There may be some prior art that can be stolen from servo/servo#7546 .

@softprops
Copy link
Author

going to take a stab at this this weekend. thanks for the links

@cassiersg
Copy link
Contributor

NB: for the mod re-ordering, we need to take exported macros into account...

@cassiersg
Copy link
Contributor

There might be some things to take from Wafflespeanut/rust-sorty.

@softprops
Copy link
Author

cool, I started looking at this again last night. look for pr soon

@rushmorem
Copy link

I would also like to see reorder_imports order extern crate declarations as well.

@jonhoo
Copy link
Contributor

jonhoo commented Sep 22, 2017

Now that Rust RFC 2126 has landed, all use paths will have a more rigid structure. In particular, it will now be much easier to determine whether a use is for an std path (if it starts with std::), for a path in the current crate (if it starts with crate::), or for some other external crate (if it starts with anything else).

Given that, maybe it's now doable to have use ordering that also separates standard library imports, local imports, and external crate imports!

@sanmai-NL
Copy link

sanmai-NL commented Sep 22, 2017

@jonhoo: In effect, standard library imports will now be adjacent after a global sort already. So I'd consider a feature to discern path types in the sorting logic a nice-to-have, while I consider automatic sorting and normalizing of imports very important. Agree?

Update:
I found out we have a bunch of sorting features already, for use declarations:

  1. https://github.com/rust-lang-nursery/rustfmt/blob/master/Configurations.md#reorder_imported_names
  2. https://github.com/rust-lang-nursery/rustfmt/blob/master/Configurations.md#reorder_imports
  3. https://github.com/rust-lang-nursery/rustfmt/blob/master/Configurations.md#reorder_imports_in_group

Could the issue be updated to reflect where the gaps remain?

@jonhoo
Copy link
Contributor

jonhoo commented Sep 22, 2017

@sanmai-NL I'd like them to not just be adjacent, but also to be separated from the main list of imports. That is, I want three "blocks" of use statements: std, crate, and others. If we just used normal lexiographic sorting, std and crate would appear somewhere in the middle of the use list.

That said, I do completely agree that this is a secondary concern, and that the primary goal should be to have sorting+normalizing.

In terms of existing features vs new features, here's what I would like (though I assume there'll be some contention here):

  • reorder_imported_names should be true by default
  • reorder_imports should be true by default
  • a new config join_imports should be added, which, if present, will place all use statements in a single list, implicitly enable reorder_imports, and thus re-order them
  • reorder_imports_in_group becomes irrelevant (or rather, equivalent to reorder_imported_names) when join_imports is true.

@sanmai-NL
Copy link

I personally would like to enforce that one name is imported per line. Similar to some Python styles. My motivation is that importing multiple names per line adds to both the writing and reading time of code, in modern IDEs. A lesser argument is that such import lines are harder to grep against.

@jonhoo
Copy link
Contributor

jonhoo commented Sep 22, 2017

@sanmai-NL so if I understand you correctly, you'd like separate_imported_names, which turns

use foo::{Bar, Baz};

into

use foo::Bar;
use foo::Baz;

? Seems very verbose for me, but as a config option I could see it. Seems potentially unrelated to this particular issue though?

@sanmai-NL
Copy link

sanmai-NL commented Sep 22, 2017

@jonhoo: It's generally unrelated to sorting indeed, but related as in related to normalizing imported names. If a programmer normalizes import lines like this, he or she wouldn't prioritize any features related to improving import statements that import multiple names. So depending on how many programmers support what (sub)features, this feature request influences prioritization of work.

When is the verbosity a problem? I’m only rarely confronted with my import statements unnecessarily when coding (I put them at the top of module files, not in deeper scopes), so is it a real issue? Whenever a programmer removes an imported name, its costs more time to revise multi-name import statements than it does to kill a line in IDEs/editors (simple key combo). These small time wastes may add up in larger projects. Sometimes the intuition in programmers to make all code more terse and elegant is counterproductive I think.

@jonhoo
Copy link
Contributor

jonhoo commented Sep 22, 2017

I'm not saying it's a problem, just that it's not my personal style. In particular if the import list ends up becoming longer than one screenful (which I could totally see if becoming for particularly complex pieces of code). And the advantage of the use being easily grep-able is lower priority for me. Once we go to nested imports though, then it becomes really tricky, and some flattening may be nice.

The title of this issue is "order stacks of imports and/or pub mod delcarations [sic]", which I would say precludes dealing with ordering of names per import. We could change the issue title, but I think this'd be worth opening a new issue for. I don't think it's reasonable to completely remove the ability to not flatten use imports, and therefore the two features can be developed in isolation.

@sanmai-NL
Copy link

To be clear, I wasn’t implying the verbosity thing must be an objective (‘serious’) problem, just asking when you would find it a problem yourself.

I agree with you.

@jonhoo
Copy link
Contributor

jonhoo commented Sep 22, 2017

@sanmai-NL well, it's tricky, because I also prefer when my imports aren't grouped, but only up to a point (and I don't know exactly where that point is). In general, if I have more than 3-4 lines for one package, I'd want them to be grouped. I'd also dislike it if the list of imports is long enough that I can't easily scan what modules the file depends on, which'd probably be around half a screenful.

@nrc nrc added this to the 1.0 milestone Nov 2, 2017
@nrc
Copy link
Member

nrc commented Nov 24, 2017

This is implemented, but buggy. Closing this issue.

@nrc nrc closed this as completed Nov 24, 2017
@nrc
Copy link
Member

nrc commented Nov 24, 2017

See also #2185

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

7 participants