Navigation Menu

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

fix(eslint-plugin): [member-ordering] order literal names correctly in #4054

Merged
merged 1 commit into from Nov 17, 2021
Merged

fix(eslint-plugin): [member-ordering] order literal names correctly in #4054

merged 1 commit into from Nov 17, 2021

Conversation

avaly
Copy link
Contributor

@avaly avaly commented Oct 25, 2021

PR Checklist

Overview

Updated the sorting algorithm to consider the raw string values of identifiers.

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @avaly!

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.

@nx-cloud
Copy link

nx-cloud bot commented Oct 25, 2021 •
edited

Nx Cloud Report

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

📂 See all runs for this branch

Status Command
#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 test @typescript-eslint/eslint-plugin-tslint
#000000 Node 12.x - nx test @typescript-eslint/eslint-plugin
#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 run-many --target=typecheck --all --parallel
#000000 Node 12.x - nx test @typescript-eslint/parser
#000000 Node 12.x - nx test @typescript-eslint/experimental-utils
#000000 Node 12.x - nx test @typescript-eslint/scope-manager
#000000 Node 12.x - nx test @typescript-eslint/visitor-keys
#000000 Node 12.x - nx test @typescript-eslint/typescript-estree
#000000 Node 14.x - nx test @typescript-eslint/parser
#000000 Node 14.x - nx test @typescript-eslint/experimental-utils
#000000 Node 12.x - nx run-many --target=build --all --parallel --exclude website --exclude website
#000000 Node 14.x - nx test @typescript-eslint/scope-manager
#000000 Node 14.x - nx test @typescript-eslint/visitor-keys

Sent with 💌 from NxCloud.

@codecov
Copy link

codecov bot commented Oct 25, 2021 •
edited

Codecov Report

Merging #4054 (4f5b865) into master (5a326e4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4054   +/-   ##
=======================================
  Coverage   93.36%   93.36%           
=======================================
  Files         152      152           
  Lines        8047     8053    +6     
  Branches     2584     2586    +2     
=======================================
+ Hits         7513     7519    +6     
  Misses        179      179           
  Partials      355      355
Flag Coverage Δ
unittest 93.36% <100.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.33% <100.00%> (+0.31%) ⬆️

JoshuaKGoldberg
JoshuaKGoldberg requested changes Oct 27, 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.

LGTM, thanks! Just requesting a little more unit testing.

packages/eslint-plugin/src/rules/member-ordering.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/tests/rules/member-ordering.test.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 27, 2021
@avaly avaly force-pushed the fix/member-ordering-literal-names branch from 5715002 to f60c402 Compare October 27, 2021 09:09
@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Nov 1, 2021
@bradzacher bradzacher added the bug Something isn't working label Nov 11, 2021
JoshuaKGoldberg
JoshuaKGoldberg previously approved these changes Nov 17, 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.

Super, thanks -- and sorry about the delay!

armano2
armano2 previously approved these changes Nov 17, 2021
View reviewed changes
@bradzacher
Copy link
Member

bradzacher commented Nov 17, 2021 •
edited

please resolve the conflict (we just merged an older PR for this same rule) - then we can merge this in!

@bradzacher bradzacher added 1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready awaiting response Issues waiting for a reply from the OP or another party labels Nov 17, 2021
@avaly avaly dismissed stale reviews from armano2 and JoshuaKGoldberg via 4f5b865 November 17, 2021 07:52
@avaly avaly force-pushed the fix/member-ordering-literal-names branch from f60c402 to 4f5b865 Compare November 17, 2021 07:52
@netlify
Copy link

netlify bot commented Nov 17, 2021 •
edited

✔️ Deploy Preview for typescript-eslint ready!

🔨 Explore the source changes: 4f5b865

🔍 Inspect the deploy log: https://app.netlify.com/sites/typescript-eslint/deploys/6194b4aa2cac0b0008f6223d

😎 Browse the preview: https://deploy-preview-4054--typescript-eslint.netlify.app

@avaly
Copy link
Contributor Author

avaly commented Nov 17, 2021

@bradzacher I resolved the git conflict.

bradzacher
bradzacher approved these changes Nov 17, 2021
View reviewed changes
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

sweet! thanks!

@bradzacher bradzacher changed the title fix(eslint-plugin): order literal names correctly in member-ordering fix(eslint-plugin): [member-ordering] order literal names correctly in Nov 17, 2021
@bradzacher bradzacher merged commit d57141a into typescript-eslint:master Nov 17, 2021
@avaly avaly deleted the fix/member-ordering-literal-names branch November 17, 2021 08:20
This was referenced Nov 30, 2021
Open
Open
Open
Open
Open
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bradzacher bradzacher bradzacher approved these changes

@armano2 armano2 armano2 left review comments

@JoshuaKGoldberg JoshuaKGoldberg Awaiting requested review from JoshuaKGoldberg

Assignees
No one assigned
Labels
1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready awaiting response Issues waiting for a reply from the OP or another party bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[member-ordering] Properties with literal keys are wrongly reported in alphabetical order
4 participants
@avaly @bradzacher @armano2 @JoshuaKGoldberg