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/sliding imports #5383

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

S-Vinter
Copy link

Once upon a time, when I was a junior developer in this c team, I had a fanatic team leader, who has preached his code style on us.
One of his principles refers to the order of the includes at the top of each file; instead of ordering them alphabetically or whatever, we had to sort them by the total length of each path:

#include <linux/bio.h>
#include <linux/uio.h>
#include <linux/blkdev.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/sched/task_stack.h>

Then, me and another friend moved to another team, which code in rust. Everything in this language was more than perfect, besides the lack of the "sliding includes". So, after a search for the right configuration, i decided to code this feature to the rustfmt

@ytmimi
Copy link
Contributor

ytmimi commented Jun 13, 2022

Thanks for the PR.

We'll certainly consider these changes. I haven't had a chance to to review yet, but could you fix the merge conflict in test/config/small_tabs.toml when you get the chance. I think the issue is that we recently removed some configuration options (ref: #5357). Here are the options that were removed:

report_todo = "Always"
report_fixme = "Never"

@calebcartwright
Copy link
Member

I think we should spend some time discussing first before jumping back to the implementation.

I'm not inherently opposed to considering some additional ordering strategies, but there's a lot of additional factors that needed to be talked through.

First and foremost, reorder_imports is a stable configuration option, so we absolutely cannot simply just change that from a boolean option to an enum based option. If we were to support multiple strategies than an enum based option would be the way to go, but we'd have to do so in a way that doesn't break backwards compatibility.

Additionally, I've only ever seen single import directives in C and C++ and I'm not sure if more complex structures are even syntactically valid, but with Rust we'll absolutely have to account for such more complex structures, such as import statements containing trees.

Such an addition would also need to be extensively tested with coverage of such scenarios, and also vectored against the other related import configuration options to validate their interactions

@ytmimi
Copy link
Contributor

ytmimi commented Jun 15, 2022

Agreed that since this is a stable option we need to take additional care to maintain backwards compatibility. To that end I feel like we could implement FromStr on ReorderImports and map:

  • "true" -> "Alphabetically"
  • "false" -> "Off".

Additionally, I've only ever seen single import directives in C and C++ and I'm not sure if more complex structures are even syntactically valid, but with Rust we'll absolutely have to account for such more complex structures, such as import statements containing trees.

Handling the trees seems like a much more challenging issue. Not entirely sure how we'd handle that right now.

Such an addition would also need to be extensively tested with coverage of such scenarios, and also vectored against the other related import configuration options to validate their interactions

Agreed that solid test coverage for the new configuration options is going to be crucial!

@ytmimi ytmimi added the p-low label Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants