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

Jump to bottom

feat(eslint-plugin): [member-ordering] add option to sort case insensitive #3896

Merged
merged 8 commits into from Nov 17, 2021
Merged

feat(eslint-plugin): [member-ordering] add option to sort case insensitive #3896

merged 8 commits into from Nov 17, 2021

Conversation

VincentRoth
Copy link
Contributor

Hello,

These PR adds 'alphabetically-ci' to order option for member-ordering rule.
It keeps the existing 'alphabetically' to avoid any breaking change.
It also split the member-ordering test in 3 files as the number of file line increases quite quickly: regular tests, alphabetically order tests and alphabetically-ci order tests.

The 'alphabetically-ci' order checks for member sort within group like 'alphabetically' does but in a case insensitive way.

With 'alphabetically' order value, the following is correct:

interface Foo {
  B : b;
  a : b;
}

With 'alphabetically-ci' order value, the following is correct:

interface Foo {
  a : b;
  B : b;
}

Part of #3380

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @VincentRoth!

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. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@VincentRoth VincentRoth changed the title feat(eslint-plugin): Member ordering order case insensitive feat(eslint-plugin): Member ordering - order case insensitive Sep 17, 2021
@codecov
Copy link

codecov bot commented Sep 17, 2021 •
edited

Codecov Report

Merging #3896 (9257fee) into master (6af7ca7) will increase coverage by 0.00%.
The diff coverage is 90.00%.

@@           Coverage Diff           @@
##           master    #3896   +/-   ##
=======================================
  Coverage   93.33%   93.33%           
=======================================
  Files         152      152           
  Lines        8026     8030    +4     
  Branches     2575     2577    +2     
=======================================
+ Hits         7491     7495    +4     
  Misses        180      180           
  Partials      355      355
Flag Coverage Δ
unittest 93.33% <90.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ackages/eslint-plugin/src/rules/member-ordering.ts 93.02% <90.00%> (+0.22%) ⬆️

@nx-cloud
Copy link

nx-cloud bot commented Sep 17, 2021 •
edited

Nx Cloud Report

CI ran the following commands for commit 9257fee. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 Node 16.x - nx test @typescript-eslint/eslint-plugin-tslint
#000000 Node 16.x - nx test @typescript-eslint/eslint-plugin
#000000 Node 12.x - nx test @typescript-eslint/eslint-plugin-tslint
#000000 Node 12.x - nx test @typescript-eslint/eslint-plugin
#000000 Node 12.x - nx test @typescript-eslint/eslint-plugin-internal
#000000 Node 12.x - nx test @typescript-eslint/eslint-plugin-tslint
#000000 Node 12.x - nx test @typescript-eslint/eslint-plugin
#000000 Node 14.x - nx test @typescript-eslint/eslint-plugin-tslint
#000000 Node 14.x - nx test @typescript-eslint/eslint-plugin
#000000 Node 12.x - nx run-many --target=typecheck --all --parallel
#000000 Node 12.x - nx run-many --target=build --all --parallel
#000000 Node 16.x - nx test @typescript-eslint/parser
#000000 Node 16.x - nx test @typescript-eslint/experimental-utils
#000000 Node 16.x - nx test @typescript-eslint/scope-manager
#000000 Node 16.x - nx test @typescript-eslint/visitor-keys
#000000 Node 16.x - nx test @typescript-eslint/typescript-estree
#000000 Node 16.x - nx run-many --target=build --all --parallel
#000000 Node 12.x - nx run-many --target=build --all --parallel
#000000 Node 12.x - nx test @typescript-eslint/parser
#000000 Node 14.x - nx test @typescript-eslint/parser

Sent with 💌 from NxCloud.

@bradzacher bradzacher added the enhancement: plugin rule option New rule option for an existing eslint-plugin rule label Sep 20, 2021
JoshuaKGoldberg
JoshuaKGoldberg requested changes Oct 16, 2021
View reviewed changes
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.

Code looks great, thanks!

Just requesting changes on the order name and trimming down the new tests.

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 Oct 16, 2021
@VincentRoth
Copy link
Contributor Author

I push the following changes, as requested :

  • order option 'alphabetically-case-insensitive' instead of 'alphabetically-ci'
  • reduce test number to test only case insensitive ordering

JoshuaKGoldberg
JoshuaKGoldberg previously approved these changes Oct 18, 2021
View reviewed changes
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.

Thanks!

@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Oct 19, 2021
@bradzacher bradzacher added the 1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready label Oct 29, 2021
@bradzacher
Copy link
Member

@VincentRoth - could you please resolve the conflict?
as soon as that's resolved we can merge this in!

@VincentRoth
Merge branch 'master' of github.com:typescript-eslint/typescript-esli…
9257fee 
…nt into member-ordering-order-case-insensitive
@VincentRoth
Copy link
Contributor Author

@bradzacher Hello, conflicts resolved

@bradzacher bradzacher changed the title feat(eslint-plugin): Member ordering - order case insensitive feat(eslint-plugin): [member-ordering] add option to sort case insensitive Nov 17, 2021
@bradzacher bradzacher merged commit e3533d5 into typescript-eslint:master Nov 17, 2021
This was referenced Nov 30, 2021
Open
Open
This was referenced Dec 2, 2021
Open
Open
Open
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bradzacher bradzacher bradzacher approved these changes

@JoshuaKGoldberg JoshuaKGoldberg JoshuaKGoldberg left review comments

Assignees
No one assigned
Labels
1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready enhancement: plugin rule option New rule option for an existing eslint-plugin rule
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@VincentRoth @bradzacher @JoshuaKGoldberg