Skip to content

Repo: Enable alphabetical (natural) sorting of fields #8479

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

Closed
JoshuaKGoldberg opened this issue Feb 15, 2024 · 8 comments · Fixed by #10189 or #10298
Closed

Repo: Enable alphabetical (natural) sorting of fields #8479

JoshuaKGoldberg opened this issue Feb 15, 2024 · 8 comments · Fixed by #10189 or #10298
Assignees
Labels
locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. repo maintenance things to do with maintenance of the repo, and not with code/docs team assigned A member of the typescript-eslint team should work on this.

Comments

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Feb 15, 2024

Suggestion

Spinning out of #8477 (comment): we're interested in maybe enabling https://eslint-plugin-perfectionist.azat.io/configs/recommended-natural or similar to enforce alphabetical sorting of fields. That likely includes methods and properties on classes, interfaces, objects, type aliases, and so on.

For reference I've used that rule in my http://github.com/JoshuaKGoldberg/create-typescript-app for quite a while now and have been very happy with it.

@JoshuaKGoldberg JoshuaKGoldberg added triage Waiting for team members to take a look repo maintenance things to do with maintenance of the repo, and not with code/docs labels Feb 15, 2024
@bradzacher bradzacher added accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for team members to take a look labels Feb 15, 2024
@JoshuaKGoldberg
Copy link
Member Author

JoshuaKGoldberg commented Mar 5, 2024

Blocked on azat-io/eslint-plugin-perfectionist#111. We sometimes use comments to delineate specific members of enums.

Edit: no longer ✅

@JoshuaKGoldberg JoshuaKGoldberg added blocked by external API Blocked by a tool we depend on exposing an API, such as TypeScript's Type Relationship API and removed accepting prs Go ahead, send a pull request that resolves this issue labels Mar 5, 2024
@JoshuaKGoldberg JoshuaKGoldberg added accepting prs Go ahead, send a pull request that resolves this issue and removed blocked by external API Blocked by a tool we depend on exposing an API, such as TypeScript's Type Relationship API labels Mar 14, 2024
@JoshuaKGoldberg JoshuaKGoldberg self-assigned this Mar 14, 2024
@abrahamguo
Copy link
Contributor

abrahamguo commented Mar 21, 2024

@JoshuaKGoldberg Here is some info that I discovered while working on #8726, that you might find helpful when you begin working on this!

  • In eslint.config.mjs, it seems like the pattern is to // register all of the plugins up-front. However, because of an interesting coincidence, this breaks for eslint-plugin-perfectionist:
    For the six existing ESLint plugins where we are using a preset (@typescript-eslint, eslint-plugin-eslint-plugin, jsdoc, jsx-a11y, react-hooks and react), it is unnecessary to register them up front, since those plugins are registered again in the preset. However, it does not break anything either.
    For eslint-plugin-perfectionist, it does break to register it up front in addition to using a preset. The reason is because eslint-plugin-perfectionist has two completely separate JS files — an ESM build and a CJS build. When we import eslint-plugin-perfectionist into eslint.config.mjs, we pull in the ESM build, but inside compat.config() when ESLint pulls in the plugin, it requires it, thus pulling in the CJS build. This causes ESLint to complain that two distinct object references are both trying to register themselves under the name eslint-plugin-perfectionist. By coincidence, none of the six existing double-registered ESLint plugins have this issue.

  • As mentioned in Bug(repo): yarn lint-fix in this repository doesn't work on the eslint-plugin package #8733, yarn lint-fix does not work on packages/eslint-plugin. I was unable to determine the root cause of this issue.

  • Several files in the root of the repo were either not linted (.github, tools), or not properly ignored from linting (.nx/cache, .yarn/releases)

  • yarn lint was very slow due to the build step, which seemed unnecessary for this task. Therefore, I ran eslint directly with caching and progress (DEBUG=eslint:flat-eslint npx eslint . --cache) which seemed much more performant for this task. At times, when autofixing, I had to increase Node's memory with --max-old-space-size.

  • Pretty obvious, but I autofixed one rule at a time in order to easily review changes.

  • At times, when autofixing, some unrelated things were wrongly autofixed, such as wrongly removing necessary optional chaining or === true/false checks. I'm guessing this is related to something with typed linting, and potentially how I was running ESLint. I did not investigate the root cause, but in any case, I was able to manually revert those bad autofixes with no issues, and re-running lint showed no further issues.

@JoshuaKGoldberg JoshuaKGoldberg added team assigned A member of the typescript-eslint team should work on this. and removed accepting prs Go ahead, send a pull request that resolves this issue labels Jun 2, 2024
@JoshuaKGoldberg JoshuaKGoldberg removed their assignment Jul 6, 2024
@JoshuaKGoldberg JoshuaKGoldberg added the blocked by another issue Issues which are not ready because another issue needs to be resolved first label Jul 10, 2024
@JoshuaKGoldberg
Copy link
Member Author

Marking as blocked on the v8 launch because the main <> v8 branch merge conflicts from doing this would be infuriating. 😂

@abrahamguo
Copy link
Contributor

@JoshuaKGoldberg, just curious — what did you end up doing with .git-blame-ignore-revs (#8726 (comment))?

@JoshuaKGoldberg
Copy link
Member Author

I figured we could add that in after the PRs are merged. We won't know the full list of commit hashes until then. 🤷

@JoshuaKGoldberg
Copy link
Member Author

JoshuaKGoldberg commented Aug 23, 2024

Note from #9842 (comment): we'd like to customize the perfectionist/sort-imports groups option at some point.

Edit: #9846 (comment), too.

@JoshuaKGoldberg
Copy link
Member Author

JoshuaKGoldberg commented Aug 23, 2024

Notes, too, on bugs or feature requests to file:

@bradzacher
Copy link
Member

Suggestion @JoshuaKGoldberg - lets disable sorting on enums.
So much of our codebase is setup with enums sorted by value and sorting by label breaks that really nice value ordering.
EG:

enum BagOfValues {
  Foo = 1,
  Bar = 2,
  Baz = 3,
  Bang = 4,
}

Is much better than

enum BagOfValues {
  Bang = 4,
  Bar = 2,
  Baz = 3,
  Foo = 1,
}

Or for a real example:

const enum Readonlyness {
/** the type cannot be handled by the function */
UnknownType = 1,
/** the type is mutable */
Mutable = 2,
/** the type is readonly */
Readonly = 3,
}

Is much better than

const enum Readonlyness {
/** the type is mutable */
Mutable = 2,
/** the type is readonly */
Readonly = 3,
/** the type cannot be handled by the function */
UnknownType = 1,
}

@github-actions github-actions bot added the locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. label Nov 12, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. repo maintenance things to do with maintenance of the repo, and not with code/docs team assigned A member of the typescript-eslint team should work on this.
Projects
None yet
3 participants