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

Remove sort-imports rule #221

Merged
merged 1 commit into from Mar 22, 2022
Merged

Remove sort-imports rule #221

merged 1 commit into from Mar 22, 2022

Conversation

keithamus
Copy link
Member

sort-imports seems to have a pretty inscrutable algorithm for how it sorts. It's not based on the import specifier, and seems to be based on bindings.

We see a lot of complaints about this rule:

  • The order is difficult to work out, making it high cognitive load to fix (or low cognitive load and multiple passes at running the lint rule until you do what it tells you)
  • It makes PR diffs messier as adding a binding can change the order required
  • It's not auto-fixable (which is exaggerated by the fact that the order is difficult to work out)
  • There seems to be no observable benefit to having imports sorted.

We added sort-imports in https://github.com/github/github/pull/51096 with a slew of other changes around import rules to catch bugs. However sort-imports didn't have good justification in isolation. Given the lack of justification originally, and given the amount of pain it causes today, I think it's a good idea to simply remove this rule.

@keithamus keithamus requested a review from a team as a code owner March 22, 2022 11:14
Copy link
Contributor

@koddsson koddsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎩
😄 👍🏻

@keithamus
Copy link
Member Author

     🤠
  🤠🤠🤠
🤠  🤠  🤠
👍  🤠  👍
  🤠   🤠
  🤠   🤠
  👢   👢

@keithamus keithamus merged commit d882a8e into main Mar 22, 2022
@keithamus keithamus deleted the kill-sort-imports-with-fire branch March 22, 2022 11:20
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.

None yet

2 participants