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

fix(eslint-plugin): [member-ordering] report alphabetical sorting for all groups instead of just the first failing group #8263

Merged
merged 11 commits into from Mar 6, 2024

Conversation

arka1002
Copy link
Contributor

@arka1002 arka1002 commented Jan 16, 2024

PR Checklist

Overview

  1. Previously, if a group of MemberType was unsorted, then it was reported ignoring the rest of the MemberTypes, now all of them are reported.
  2. Previously, if the MemberTypes weren't in order wrt config, alphabetic sort was avoided, now both takes place.

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @arka1002!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

Copy link

netlify bot commented Jan 16, 2024

Deploy Preview for typescript-eslint failed.

Name Link
🔨 Latest commit 86af17b
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/65e0ab074770a900080f42d5

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Seems reasonable on a read-through, but I'm having a hard time understanding the added code. Which means future folks with less context would probably have a harder time. What do you think about the clarification + cleanup comments?

Btw, emphasizing from a comment: this rule is inherently pretty tricky and complex. So I think any changes will be that way too. Nice job getting it to work! 👏

packages/eslint-plugin/src/rules/member-ordering.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/member-ordering.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/member-ordering.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/member-ordering.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/member-ordering.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/member-ordering.ts Outdated Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Feb 1, 2024
@JoshuaKGoldberg
Copy link
Member

fixes: #8100

PR Checklist

...

Btw, no need to put the fixes: #... on top of the PR description. It's in the checklist. 🙂

@github-actions github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Feb 6, 2024
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

⚡ this is looking really snazzy! I'm happy with how it's coming along, and think I understand it a lot better now thanks to the new structures & comments.

Mostly requesting changes on some small touchups. I think this is getting pretty close to merge-ready!

Guy wearing a tie shooting blue lightning out of his hands, calmly

Comment on lines 1023 to 1027
return grouped
.map(groupMember =>
checkAlphaSort(groupMember, order as AlphabeticalOrder),
)
.some(result => !result);
Copy link
Member

Choose a reason for hiding this comment

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

[Performance] Why waste time run lot iteration when few do trick?

Suggested change
return grouped
.map(groupMember =>
checkAlphaSort(groupMember, order as AlphabeticalOrder),
)
.some(result => !result);
return grouped.some(
groupMember =>
!checkAlphaSort(groupMember, order as AlphabeticalOrder),
);

More helpfully: .map does a loop over everything in grouped, then .some bails out as soon as a result is found. So we can combine the two into one to bail out early if possible.

Copy link
Contributor Author

@arka1002 arka1002 Feb 29, 2024

Choose a reason for hiding this comment

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

Yeah, using .some is a part of the problem of the issue. Currently before this pr, the rule only run alphabetical sort on a particular group, if that group was sorted, then only it checked the next group(which is the early bailing out due to .some), else the next group remains unchecked.

An example test - https://github.com/typescript-eslint/typescript-eslint/pull/8263/files#diff-28e99227546efaad4ed38d47dcd8746943b60b09b2095a68ec4209856d709e1fR128-R129

But yeah, I removed .some, I think it isn't necessary.

packages/eslint-plugin/src/rules/member-ordering.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/member-ordering.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/member-ordering.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/member-ordering.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/member-ordering.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/member-ordering.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/member-ordering.ts Outdated Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Feb 22, 2024
@github-actions github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Feb 29, 2024
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

🙌 Whoop! This looks wonderful - thanks for working with me on it.

Since I've looked at it so much, I'll leave it open as 1 approval in the hopes another team member can get to it.

@JoshuaKGoldberg JoshuaKGoldberg added the 1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready label Mar 5, 2024
Copy link
Member

@auvred auvred left a comment

Choose a reason for hiding this comment

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

The changes looks great! 🚀

@JoshuaKGoldberg JoshuaKGoldberg merged commit c06ce1a into typescript-eslint:main Mar 6, 2024
56 of 60 checks passed
@flohw
Copy link

flohw commented Mar 12, 2024

Just updated and tested. This is a huge improvement to have all the errors at once.

Thank you again for the fix and reviews! 🙂

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[member-ordering] report alphabetical sorting for all groups instead of just the first failing group
4 participants