Skip to content

Turn on rustfmt merge_imports#284

Closed
LLFourn wants to merge 1 commit intobitcoindevkit:masterfrom
LLFourn:rustfmt_merge_imports
Closed

Turn on rustfmt merge_imports#284
LLFourn wants to merge 1 commit intobitcoindevkit:masterfrom
LLFourn:rustfmt_merge_imports

Conversation

@LLFourn
Copy link
Collaborator

@LLFourn LLFourn commented Feb 11, 2021

Was brought up here: #279 (comment)

So I made PR to see what it would look like and to see if we have overwhelming support for it. I think I prefer merge imports as it's easier to see what top level modules are being imported. It does add some more LoC funnily enough.

Voice your opinion. There are also other options we could turn on in rustfmt. See here which could be added to the PR.

and set edition = "2018" which I guess you are meant to do.
@notmandatory
Copy link
Member

I prefer the out of the box imports style since it's what most other projects use, unless there's a compelling reason to change it. That said, the merged imports are easy enough to read and rustfmt does all the work, so if others are strongly for it I'm not strongly against.

@RCasatta
Copy link
Member

Personally, I slightly prefer without nested imports

@tcharding
Copy link
Contributor

I prefer non-merged imports. There are trade offs, the main ones being IMO

pro of merge imports:

  • Easier tooling support
  • Less cluttered

cons of merged imports

  • diffs are bigger
  • use statement changes in the diff often cause merge conflicts

Personally I think the merge conflict thing is the biggest consideration hence my preference for non-merged imports.

Sorry @LLFourn if my comment on the PR that you link to was misleading, I already had an opinion but did not air it then.

@afilini
Copy link
Member

afilini commented Feb 13, 2021

I can't really describe why (maybe it's just that I'm more used to them), but I also like the current non-merged style better..

@LLFourn
Copy link
Collaborator Author

LLFourn commented Feb 14, 2021

Thanks all.

@LLFourn LLFourn closed this Feb 14, 2021
@LLFourn LLFourn deleted the rustfmt_merge_imports branch February 14, 2021 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants