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

[unstable option] imports_layout #3361

Open
scampi opened this issue Feb 13, 2019 · 24 comments
Open

[unstable option] imports_layout #3361

scampi opened this issue Feb 13, 2019 · 24 comments
Labels
unstable option tracking issue of an unstable option

Comments

@scampi
Copy link
Contributor

scampi commented Feb 13, 2019

Tracking issue for unstable option: imports_layout

@scampi scampi added the unstable option tracking issue of an unstable option label Feb 13, 2019
@scampi scampi changed the title [unstable option] unstable option: imports_layout [unstable option] imports_layout Feb 18, 2019
@jimmycuadra
Copy link

What's necessary to stabilize this?

@scampi
Copy link
Contributor Author

scampi commented Jun 2, 2019

There are some steps described in https://github.com/rust-lang/rustfmt/blob/master/Processes.md#stabilising-an-option.
#3581 is an attempt at this.

@mzabaluev
Copy link

Why does Horizontal have to exceed max_width?
How does this behavior interact with merge_imports = true?

I think it should be possible to try to fit all import statements into max_width, replicating the path prefix for any spill-over items into the next line(s).

@mzabaluev
Copy link

One good reason to prefer the Horizontal layout (and stabilize this setting after the behavioral aspects are ironed out) is to keep the imports searchable with grep-like tools that don't easily match across multiple lines.

@andreastt
Copy link

andreastt commented Oct 19, 2019

A counter-argument for Vertical sorting is that it makes rebases significantly easier.

I recently had to do a rebase of a fairly complicated refactoring that moved lots of types around, and git rebase tools for in-line changes is not great.

I would like to use this feature in mozilla-central once it’s stablised.

@vn971
Copy link

vn971 commented May 22, 2020

I would love independent imports:

use foo::{
    aaaaaaaaaaaaaaaaaa, bbbbbbbbbbbbbbbbbb, cccccccccccccccccc, dddddddddddddddddd,
    eeeeeeeeeeeeeeeeee, ffffffffffffffffff,
};

=>

use foo::aaaaaaaaaaaaaaaaaa;
use foo::bbbbbbbbbbbbbbbbbb;
use foo::cccccccccccccccccc;
use foo::dddddddddddddddddd;
use foo::eeeeeeeeeeeeeeeeee;
use foo::ffffffffffffffffff;

As a possible alternative, I find this style super-friendly to git, immediately readable to my tastes, easy to play around with commenting-uncommenting.

Is it possible to have it as an independent alternative option?

UPDATE: this has been implemented in the (currently unstable) option imports_granularity 🎉

@dtolnay
Copy link
Member

dtolnay commented Jun 11, 2020

I think "independent imports" (#3361 (comment)) is outside of what this option is for, and is a discussion for #3362 instead. It's already been brought up there in #3362 (comment).

AIUI the two options are orthogonal and the distinction is as follows: you can think of merge_imports as determining the tokens of our imports i.e. how/whether they should be grouped i.e. which parts of which specific paths should go inside of the same curly braces; while imports_layout determines the whitespace of each import after the point that the tokens have already been decided.

@Lokathor
Copy link

Lokathor commented Aug 2, 2020

Shouldn't "mixed' be called "compressed" to match other similar options?

@calebcartwright

This comment has been minimized.

@Fishrock123
Copy link

Is this the same thing as imports_granularity? What kind of timeline / issues are there for stabilizing that?

@bartlomieju
Copy link

Deno project is very interested in this setting as well (because we only use latests stable Rust toolchain).

We'd prefer to format to have a single import per line (which is is very git diff friendly).

@9999years
Copy link

Is this the same thing as imports_granularity? What kind of timeline / issues are there for stabilizing that?

I've opened #4991 to track imports_granularity.

facebook-github-bot pushed a commit to facebook/sapling that referenced this issue Oct 19, 2021
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
@Progdrasil
Copy link

Is there anything missing to stabilize this option?

@9999years
Copy link

@Progdrasil One of the big things missing is dealing with comments in imports; see #4991 (comment)

@ytmimi
Copy link
Contributor

ytmimi commented Jul 20, 2022

linking #3286

@blyxyas
Copy link
Member

blyxyas commented Sep 17, 2022

The option LimitedHorizontalVertical isn't documented

@ytmimi
Copy link
Contributor

ytmimi commented Sep 18, 2022

@blyxyas PRs for doc improvements are always welcome 😁

@blyxyas
Copy link
Member

blyxyas commented Sep 18, 2022

I haven't made a PR because I don't know what it does
Sorry

@ytmimi
Copy link
Contributor

ytmimi commented Sep 19, 2022

@blyxyas no worries. I also couldn't have told you what it did before looking into it. From what I can tell it's meant to let you control the width at which imports wrap from being laid out horizontally to being laid out vertically, however I don't think we support parsing config options that take a value, and that might be why its not documented.

imports_layout config option maps to the ListTactic enum.

imports_layout: ListTactic, ListTactic::Mixed, false, "Item layout inside a import block";

/// Formatting tactic for lists. This will be cast down to a
/// `DefinitiveListTactic` depending on the number and length of the items and
/// their comments.
#[config_type]
pub enum ListTactic {
/// One item per row.
Vertical,
/// All items on one row.
Horizontal,
/// Try Horizontal layout, if that fails then vertical.
HorizontalVertical,
/// HorizontalVertical with a soft limit of n characters.
LimitedHorizontalVertical(usize),
/// Pack as many items as possible per row over (possibly) many rows.
Mixed,
}

I tried setting it from the command line and got an error

rustfmt --config="imports_layout=LimitedHorizontalVertical" 
invalid key=val pair: `imports_layout=LimitedHorizontalVertical`

rustfmt --config="imports_layout=LimitedHorizontalVertical(50)"
invalid key=val pair: `imports_layout=LimitedHorizontalVertical(50)`

The ListTactic enum is pretty general and is used internally to format many different types of lists. Maybe it makes more sense to create a new ImportsListTactic enum that exposes the same options as ListTactic except the LimitedHorizontalVertical(50) variant. Given that LimitedHorizontalVertical(_) can't be set (at least not from the command line) maybe it's not worth it to make that change.

@tgross35
Copy link
Contributor

tgross35 commented Apr 3, 2023

It seems like there are some weird interactions with the imports_granularity option. For example, with import_granularity = "Crate" and imports_layout = "Horizontal":

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

HorizontalVertical

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

Mixed (default)

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

None of those really match up with what I would expect based on the examples, it seems like it jumps to multiline too quick.

@ytmimi
Copy link
Contributor

ytmimi commented Apr 3, 2023

@tgross35 I believe the current formatting is correct based on the Nested import rules defined in the style guide

@tgross35
Copy link
Contributor

tgross35 commented Apr 3, 2023

If there are any nested imports in a list import, then use the multi-line form, even if the import fits on one line. Each nested import must be on its own line, but non-nested imports must be grouped on as few lines as possible.

I guess that does line up. Though I'm not sure why "Horizontal" breaks into a multiline group when it's not close to line length

@ytmimi
Copy link
Contributor

ytmimi commented Apr 3, 2023

Though I'm not sure why "Horizontal" breaks into a multiline group when it's not close to line length

@tgross35 From the style guide:
"If there are any nested imports in a list import, then use the multi-line form, even if the import fits on one line."

@tgross35
Copy link
Contributor

tgross35 commented Apr 3, 2023

It was just my initial understanding that Horizontal breaks the rules to keep everything on one line, e.g., in the docs it says that it may exceed line length. There just isn't an example with nested modules in the docs. (fwiw, I don't have any problem with the behavior, it just didn't line up with my impression from the docs)

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

No branches or pull requests