Added a new property required to SortedOrderConfig.
Added a corresponding error message id for when a rule fails because of required being set to 'first' or 'last'.

If required is 'first', then before any sorting/grouping configurations, we ensure that the members array has all its required items first and all optional items afterwards. Afterwards, we perform the remaining sorting/grouping checks on the required subset and the optional subset.

asdf93074 added5 commits

November 10, 2022 17:25

fix(eslint-plugin): [member-ordering] add requiredFirst as an option …

00d0066

…which ensures that all required members appear before all optional members.

fix(eslint-plugin): [member-ordering] adding types so build passes.

c00c2a9

fix(eslint-plugin): [member-ordering] fixing types so build passes.

0920426

fix(eslint-plugin): [member-ordering] refactoring getIndexOfLastRequi…

bf90b29

…redMember to be slightly faster and adding jsdoc comments for it and isMemberOptional.

fix(eslint-plugin): [member-ordering] additional test cases and handl…

afcb6b9

…ing for them.

Copy link

nx-cloudbot commentedNov 10, 2022•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit17c0595. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 47 targets

Sent with 💌 fromNxCloud.

Copy link

Contributor

typescript-eslintbot commentedNov 10, 2022

Thanks for the PR,@asdf93074!

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

netlifybot commentedNov 10, 2022•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`17c0595`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/6384d886051118000832fa09
😎 Deploy Preview	https://deploy-preview-5965--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to yourNetlify site settings.

fix(eslint-plugin): [member-ordering] linting fix.

8c3e9d4

Copy link

codecovbot commentedNov 10, 2022•
edited
Loading

Codecov Report

Merging#5965 (17c0595) intomain (becd1f8) willincrease coverage by0.02%.
The diff coverage is100.00%.

Additional details and impacted files

@@            Coverage Diff             @@##             main    #5965      +/-   ##==========================================+ Coverage   91.24%   91.27%   +0.02%==========================================  Files         366      366                Lines       12380    12417      +37       Branches     3621     3631      +10     ==========================================+ Hits        11296    11333      +37  Misses        774      774                Partials      310      310

Flag	Coverage Δ
unittest	`91.27% <100.00%> (+0.02%)`	⬆️

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	`96.13% <100.00%> (+0.73%)`	⬆️
packages/eslint-plugin/src/util/misc.ts	`96.87% <100.00%> (+0.44%)`	⬆️

JoshuaKGoldberg reviewed

Nov 12, 2022

View reviewed changes

packages/eslint-plugin/src/rules/member-ordering.ts OutdatedShow resolvedHide resolved

fix(eslint-plugin): [member-ordering] change requiredFirst to require…

31e9a99

…d which takes first or last as a value and adding functionality to check order based on both of these along with additional tests.

asdf93074 marked this pull request as ready for review

November 15, 2022 11:23

JoshuaKGoldberg changed the title~~feat(eslint-plugin): [member-ordering] Add a requiredFirst option to ensure that all required members are declared before all optional ones.~~feat(eslint-plugin): [member-ordering] Add a required option to ensure that all required members are declared before all optional ones.

Nov 15, 2022

JoshuaKGoldberg changed the title~~feat(eslint-plugin): [member-ordering] Add a required option to ensure that all required members are declared before all optional ones.~~feat(eslint-plugin): [member-ordering] add a required option for required vs. optional member ordering

Nov 15, 2022

JoshuaKGoldberg requested changes

Nov 15, 2022

View reviewed changes

Copy link

Member

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 for sending this in@asdf93074! I'm excited to get this feature in. 🎉

There's a lot of changed code in this PR and themember-ordering rule is already a lot of lines. We should be able to trim the changes down a bit to be easier to read & review. I can take a deeper look once the refactors are done. Let me know if anything's unclear or not right!

packages/eslint-plugin/src/rules/member-ordering.ts OutdatedShow resolvedHide resolved

JoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party label

Nov 15, 2022

fix(eslint-plugin): [member-ordering] refactoring according to PR com…

ce40b8e

…ments.

bradzacher removed the awaiting responseIssues waiting for a reply from the OP or another party label

Nov 18, 2022

asdf93074 added4 commits

November 18, 2022 18:29

fix(eslint-plugin): [member-ordering] refactoring for PR and adding a…

c09d42a

…nother test case.

fix(eslint-plugin): [member-ordering] refactoring for PR.

abf6b8d

fix(eslint-plugin): [member-ordering] adding test cases for coverage …

f583399

…and removing unused code.

fix(eslint-plugin): [member-ordering] increasing coverage to pass check.

1691fa7

JoshuaKGoldberg requested changes

Nov 19, 2022

View reviewed changes

Copy link

Member

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 for making the changes@asdf93074! I actually personally find this a good deal easier to read, but maybe that's just my personal preference. If another maintainer wants to weigh in that'd be good too 😄 - I'm nervous about stomping over perfectly good code. (and to be fair, yours did work quite well before too!)

Just requesting changes on a bit more testing. Otherwise this looks good to me!

packages/eslint-plugin/src/rules/member-ordering.tsShow resolvedHide resolved

JoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party label

Nov 19, 2022

feat(eslint-plugin): [member-ordering] adding more tests to increase …

bb8444f

…coverage for isMemberOptional function.

JoshuaKGoldberg previously approved these changes

Nov 22, 2022

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Fantastic, this is looking great. Thanks for sending this in and iterating on it with me@asdf93074! 🙌

Copy link

Member

JoshuaKGoldberg commentedNov 22, 2022•
edited
Loading

I'll leave this open for a bit in case another maintainer wants to poke at it or suggest an alternative API name (which we can apply ourselves if you don't want to keep receiving extra work because of our indecision 😅). But I've set a reminder to merge it this weekend if nobody requests changes, so it'll be available in our next stable version.

Edit: see#5965 (comment), will go on Monday!

JoshuaKGoldberg added 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge and removed awaiting responseIssues waiting for a reply from the OP or another party labels

Nov 22, 2022