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

feat(eslint-plugin): [naming-convention] support the auto-accessor syntax #8084

Merged
merged 30 commits into from Feb 22, 2024

Conversation

arka1002
Copy link
Contributor

Fixes: #7823

PR Checklist

Overview

Adds auto-accessor in the accessor selector.

@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 Dec 17, 2023

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit 8f1c37d
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/65d7c4a4e20ade0008e8d990
😎 Deploy Preview https://deploy-preview-8084--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 99 (🟢 up 9 from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 98 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@arka1002 arka1002 changed the title fix: support auto-accessor for naming-convention fix(eslint-plugin): [naming-convention] support the auto-accessor syntax Dec 19, 2023
@arka1002 arka1002 marked this pull request as draft December 19, 2023 05:07
@arka1002 arka1002 marked this pull request as ready for review December 19, 2023 05:07
@arka1002
Copy link
Contributor Author

@Josh-Cena can you please take a look.

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.

Functionally looks great, thanks! Nice and straightforward. 🙂

Just requesting changes to get it documented too. Every selector option should be.

Btw, no need to ping individuals for review. We have a queue and we get to things when we can.

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Jan 4, 2024
@github-actions github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Jan 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.

Progress, thanks for working on it!

I'll defer to @bradzacher for final approval - but I think having more docs and matching test coverage to the new feature would be good.

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Jan 11, 2024
@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Jan 14, 2024
@JoshuaKGoldberg
Copy link
Member

Adding back awaiting response as we haven't been explicitly re-requested for review. But if if / when this is ready, happy to take another look!

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Jan 30, 2024
@arka1002
Copy link
Contributor Author

@JoshuaKGoldberg I don't have the button to re-request review :(
Reviewer tab ss
From my side, changes are done, this might be worth a discussion - #8084 (comment) - wdyt ?

@JoshuaKGoldberg JoshuaKGoldberg requested review from bradzacher and removed request for bradzacher February 3, 2024 22:29
@github-actions github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Feb 3, 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.

I think the direction is good - but the two new sub-groups shouldn't have overlapping matches, no?

Copy link
Member

Choose a reason for hiding this comment

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

[Testing] It's good to have created a test file for classicAccessor - but there should also be a test case file for accessor too! (so, three *.test.ts files around accessors, not two)

Copy link
Contributor Author

@arka1002 arka1002 Feb 6, 2024

Choose a reason for hiding this comment

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

Ah yes! Added here 88acaae. There are a bunch of tests in the other file too

class Ignored {
private static get some_name() {}
get IgnoredDueToModifiers() {}
}
`,
options: [
{
selector: 'default',
format: ['PascalCase'],
},
{
selector: 'accessor',
format: ['snake_case'],
modifiers: ['private', 'static'],
},
],

@JoshuaKGoldberg
Copy link
Member

I don't have the button to re-request review :(

Oh, I think I got lost in the GitHub UI - I see you did re-request review from at least me. Sorry about that!

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Feb 3, 2024
@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.

Whoohoo, this is lovely! 🙌

Thanks for working on it & iterating with us @arka1002. Really nicely done final result here.

I just have the one docs request, but nothing left from me in code. Will defer to @bradzacher. 🙂

packages/eslint-plugin/docs/rules/naming-convention.md Outdated Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added the 1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready label Feb 21, 2024
@JoshuaKGoldberg JoshuaKGoldberg requested review from bradzacher and removed request for bradzacher February 21, 2024 15:12
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.

one small change otherwise LGTM
thanks so much for working on this!

packages/eslint-plugin/src/rules/naming-convention.ts Outdated Show resolved Hide resolved
@bradzacher bradzacher merged commit f7198db into typescript-eslint:main Feb 22, 2024
59 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 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 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[naming-convention] support the auto-accessor syntax
3 participants