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

merge_imports should be a little more conservative #2982

Closed
whatisaphone opened this issue Aug 30, 2018 · 2 comments
Closed

merge_imports should be a little more conservative #2982

whatisaphone opened this issue Aug 30, 2018 · 2 comments
Labels
a-imports `use` syntax only-with-option requires a non-default option value to reproduce poor-formatting

Comments

@whatisaphone
Copy link

I turned on merge_imports and ran cargo fmt over a codebase of mine, with mixed results. Since that option is not yet stable, I thought you might be interested in some feedback.

Summary

Here is one change it made that I found useful:

-use behavior::Behavior;
-use behavior::NullBehavior;
+use behavior::{Behavior, NullBehavior};

From two lines down to one. Looking good 👍

Here is a change I found unnecessary:

-use std::io::{Read, Write};
-use std::thread;
+use std::{
+    io::{Read, Write},
+    thread,
+};

This one went from 2 lines to 4, and became a bit of a maze to read.

Possible changes

The options for merge_imports are true and false. The import porridge is always either too hot or too cold. It would be nice if there were a "just right" option. I can think of a couple possibilities for the specific implementation:

  1. Only merge imported names that do not refer to a module:

    use std::io::{Read, Write};
    use std::mem;
    use std::thread;
  2. Only merge at most a single level of names:

    use std::io::{Read, Write};
    use std::{mem, thread};

Whichever one is easier to implement or has fewest corner cases is probably the way to go.


In general, with the default options rustfmt has been very valuable to me and saved me a lot of time doing menial formatting. Thanks for working on it! 😄

@nrc nrc added poor-formatting only-with-option requires a non-default option value to reproduce labels Sep 4, 2018
@mulkieran
Copy link

Here is a change I found unnecessary:

-use std::io::{Read, Write};
-use std::thread;
+use std::{
+    io::{Read, Write},
+    thread,
+};

I actually liked this result. I think it should be possible to be prevented by a separating line between the two imports, so:

use std::io::{Read, Write};

use std::io::thread;

would remain unchanged due to the separating line. Maybe that always happens, anyway.

@ytmimi
Copy link
Contributor

ytmimi commented Jul 19, 2022

merge_imports has been deprecated for a while. The desired behavior can be produced by using imports_granularity=Module.

Input

use behavior::Behavior;
use behavior::NullBehavior;
use std::io::{Read, Write};
use std::thread;

output

use behavior::{Behavior, NullBehavior};
use std::io::{Read, Write};
use std::thread;

@ytmimi ytmimi closed this as completed Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-imports `use` syntax only-with-option requires a non-default option value to reproduce poor-formatting
Projects
None yet
Development

No branches or pull requests

4 participants