feat(eslint-plugin): [naming-convention] add support for "override" and "async" modifiers (#5310) #5610

eliasm307 · 2022-09-07T00:41:23Z

PR Checklist

Addresses an existing open issue: fixes Enhancement: [naming-convention] Add "override" modifier support #5310
That issue was marked as accepting prs
Steps in CONTRIBUTING.md were taken

Overview

Added support for "override" and "async" modifiers using existing processes

nx-cloud · 2022-09-07T00:41:25Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 3a2086d. 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 46 targets

Sent with 💌 from NxCloud.

typescript-eslint · 2022-09-07T00:41:26Z

Thanks for the PR, @eliasm307!

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.

netlify · 2022-09-07T00:41:27Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`3a2086d`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/6369727dc37b42000810d95c
😎 Deploy Preview	https://deploy-preview-5610--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 your Netlify site settings.

eliasm307 · 2022-09-07T00:43:35Z

Note the original issue (that I raised) was just for override modifier support, but I also realised async modifier support was missing so did it as part of this as it could also be useful for some of my use cases

…nd "async" modifiers (typescript-eslint#5310)

packages/eslint-plugin/src/rules/naming-convention.ts

codecov · 2022-09-07T00:56:44Z

Codecov Report

Merging #5610 (b3446d6) into main (45e2912) will decrease coverage by 0.29%.
The diff coverage is 81.25%.

❗ Current head b3446d6 differs from pull request most recent head f59a7f3. Consider uploading reports for the commit f59a7f3 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5610      +/-   ##
==========================================
- Coverage   91.59%   91.30%   -0.30%     
==========================================
  Files         369      365       -4     
  Lines       12391    12267     -124     
  Branches     3613     3585      -28     
==========================================
- Hits        11350    11200     -150     
- Misses        685      759      +74     
+ Partials      356      308      -48

Flag	Coverage Δ
unittest	`91.30% <81.25%> (-0.30%)`	⬇️

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

Impacted Files	Coverage Δ
...-plugin/src/rules/naming-convention-utils/enums.ts	`100.00% <ø> (ø)`
...plugin/src/rules/naming-convention-utils/schema.ts	`100.00% <ø> (ø)`
...kages/eslint-plugin/src/rules/naming-convention.ts	`81.10% <81.25%> (+0.01%)`	⬆️
packages/utils/src/ts-eslint/Linter.ts	`50.00% <0.00%> (-50.00%)`	⬇️
packages/utils/src/ts-eslint/SourceCode.ts	`50.00% <0.00%> (-50.00%)`	⬇️
...ges/utils/src/ast-utils/eslint-utils/predicates.ts	`51.11% <0.00%> (-48.89%)`	⬇️
packages/utils/src/ts-eslint-scope/Scope.ts	`52.17% <0.00%> (-47.83%)`	⬇️
...s/utils/src/ast-utils/eslint-utils/astUtilities.ts	`53.33% <0.00%> (-46.67%)`	⬇️
packages/utils/src/ast-utils/predicates.ts	`51.11% <0.00%> (-44.45%)`	⬇️
packages/utils/src/ast-utils/misc.ts	`33.33% <0.00%> (-33.34%)`	⬇️
... and 256 more

packages/eslint-plugin/docs/rules/naming-convention.md

packages/eslint-plugin/src/rules/naming-convention.ts

JoshuaKGoldberg

Sorry for taking so long to get to this review - but the code mostly looks great! Just a couple clarifications on docs, and cleaning up the tests to be a little more independent. Thanks! ✨

packages/eslint-plugin/tests/rules/naming-convention/naming-convention.test.ts

JoshuaKGoldberg · 2022-10-26T20:09:22Z

Ping @eliasm307 - do you still have time to work on this? No worries if not, just don't want it pending too long. 🙂

eliasm307 · 2022-10-26T20:49:08Z

Hi @JoshuaKGoldberg yeah I've been thinking about this recently but I haven't had a moment to apply the feedback. I'm busy this weekend but might have a look next weekend

…ve abstract getter change, remove override for variable selector in docs

eliasm307 · 2022-11-05T21:29:30Z

Ping @eliasm307 - do you still have time to work on this? No worries if not, just don't want it pending too long. 🙂

@JoshuaKGoldberg Thanks for the feedback! I've actioned the feedback you gave, waiting to see if the CI checks pass.

Otherwise, let me know if there is anything else.

JoshuaKGoldberg

Very close! Just one more line to get tested please 😁 thanks!

packages/eslint-plugin/src/rules/naming-convention.ts

JoshuaKGoldberg · 2022-11-07T00:06:10Z

packages/eslint-plugin/src/rules/naming-convention.ts

+        | TSESTree.TSAbstractMethodDefinitionNonComputedName,
+    ): boolean {
+      return Boolean(
+        'value' in propertyOrMemberNode &&


Looking at this again: we generally try not to use this string in checking pattern, as it's a little imprecise. We normally check for specific types. But I can see why you did it here (much less code!), and there's already an instance of it in the file.

I think it's fine to check in as-is, and we should separately look into making a lint rule internally that flags this kind of check.

I also think I'd meant to post this in the first review but forgot 😄

as it's a little imprecise

I agree, and its not great for readability also, its not really clear what the intent of the in is, in terms of what node types its trying to check for, but I went with it since it was used everywhere in that file

I can see why you did it here (much less code!)

Yeah I don't do it like this usually, but I was also surprised how short the code gets, but I'm still a fan of the type utils for better communication (but please don't ask me change it now😅)

Haha don't worry, I won't 😄

eslint-community/eslint-plugin-eslint-plugin#326

JoshuaKGoldberg

Wonderful, thanks! 🙌

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.42.1` -> `5.43.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/5.42.1/5.43.0) | | [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.42.1` -> `5.43.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/5.42.1/5.43.0) | --- ### Release Notes <details> <summary>typescript-eslint/typescript-eslint (@typescript-eslint/eslint-plugin)</summary> ### [`v5.43.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#5430-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5421v5430-2022-11-14) [Compare Source](typescript-eslint/typescript-eslint@v5.42.1...v5.43.0) ##### Bug Fixes - **eslint-plugin:** \[no-shadow] handle false positives on generics and parameters ([#5902](typescript-eslint/typescript-eslint#5902)) ([769e8c8](typescript-eslint/typescript-eslint@769e8c8)) - **eslint-plugin:** \[promise-function-async] handle keyword token ([#5907](typescript-eslint/typescript-eslint#5907)) ([f25a94f](typescript-eslint/typescript-eslint@f25a94f)) ##### Features - **eslint-plugin:** \[consistent-type-imports] support fixing to inline types ([#5050](typescript-eslint/typescript-eslint#5050)) ([75dcdf1](typescript-eslint/typescript-eslint@75dcdf1)) - **eslint-plugin:** \[naming-convention] add support for "override" and "async" modifiers ([#5310](typescript-eslint/typescript-eslint#5310)) ([#5610](typescript-eslint/typescript-eslint#5610)) ([c759da1](typescript-eslint/typescript-eslint@c759da1)) - **eslint-plugin:** \[prefer-optional-chain] support suggesting `!foo || !foo.bar` as a valid match for the rule ([#5594](typescript-eslint/typescript-eslint#5594)) ([923d486](typescript-eslint/typescript-eslint@923d486)) #### [5.42.1](typescript-eslint/typescript-eslint@v5.42.0...v5.42.1) (2022-11-07) ##### Bug Fixes - **eslint-plugin:** isTypeReadonly stack overflow ([#5875](typescript-eslint/typescript-eslint#5875)) ([#5876](typescript-eslint/typescript-eslint#5876)) ([2d9a33c](typescript-eslint/typescript-eslint@2d9a33c)) </details> <details> <summary>typescript-eslint/typescript-eslint (@typescript-eslint/parser)</summary> ### [`v5.43.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#5430-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5421v5430-2022-11-14) [Compare Source](typescript-eslint/typescript-eslint@v5.42.1...v5.43.0) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) #### [5.42.1](typescript-eslint/typescript-eslint@v5.42.0...v5.42.1) (2022-11-07) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1639 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

feat(eslint-plugin): [naming-convention] add support for "override" a…

58dbdaf

…nd "async" modifiers (typescript-eslint#5310)

eliasm307 force-pushed the naming-convention-Add-override-modifier-support branch from 7c5f850 to 58dbdaf Compare September 7, 2022 00:49

eliasm307 commented Sep 7, 2022

View reviewed changes

packages/eslint-plugin/src/rules/naming-convention.ts Outdated Show resolved Hide resolved

JoshuaKGoldberg reviewed Sep 27, 2022

View reviewed changes

packages/eslint-plugin/docs/rules/naming-convention.md Outdated Show resolved Hide resolved

packages/eslint-plugin/src/rules/naming-convention.ts Outdated Show resolved Hide resolved

JoshuaKGoldberg requested changes Sep 27, 2022

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Sep 27, 2022

apply pr feedback ie remove test case util, split tests by type, remo…

b3446d6

…ve abstract getter change, remove override for variable selector in docs

JoshuaKGoldberg requested changes Nov 7, 2022

View reviewed changes

remove async parameter logic which is impossible

f59a7f3

JoshuaKGoldberg approved these changes Nov 7, 2022

View reviewed changes

Merge branch 'main' into naming-convention-Add-override-modifier-support

3a2086d

JoshuaKGoldberg enabled auto-merge (squash) November 7, 2022 21:04

JoshuaKGoldberg mentioned this pull request Nov 7, 2022

Rule request: don't use the in operator on nodes eslint-community/eslint-plugin-eslint-plugin#326

Closed

JoshuaKGoldberg merged commit c759da1 into typescript-eslint:main Nov 7, 2022

github-actions bot locked as resolved and limited conversation to collaborators Nov 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): [naming-convention] add support for "override" and "async" modifiers (#5310) #5610

feat(eslint-plugin): [naming-convention] add support for "override" and "async" modifiers (#5310) #5610

eliasm307 commented Sep 7, 2022 •

edited

Loading

nx-cloud bot commented Sep 7, 2022 •

edited

Loading

typescript-eslint bot commented Sep 7, 2022

netlify bot commented Sep 7, 2022 •

edited

Loading

eliasm307 commented Sep 7, 2022

codecov bot commented Sep 7, 2022 •

edited

Loading

JoshuaKGoldberg left a comment

JoshuaKGoldberg commented Oct 26, 2022

eliasm307 commented Oct 26, 2022

eliasm307 commented Nov 5, 2022 •

edited

Loading

JoshuaKGoldberg left a comment

JoshuaKGoldberg Nov 7, 2022

eliasm307 Nov 7, 2022 •

edited by JoshuaKGoldberg

Loading

JoshuaKGoldberg Nov 7, 2022

JoshuaKGoldberg Nov 7, 2022

JoshuaKGoldberg left a comment

feat(eslint-plugin): [naming-convention] add support for "override" and "async" modifiers (#5310) #5610

feat(eslint-plugin): [naming-convention] add support for "override" and "async" modifiers (#5310) #5610

Conversation

eliasm307 commented Sep 7, 2022 • edited Loading

PR Checklist

Overview

nx-cloud bot commented Sep 7, 2022 • edited Loading

☁️ Nx Cloud Report

typescript-eslint bot commented Sep 7, 2022

netlify bot commented Sep 7, 2022 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

eliasm307 commented Sep 7, 2022

codecov bot commented Sep 7, 2022 • edited Loading

Codecov Report

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Oct 26, 2022

eliasm307 commented Oct 26, 2022

eliasm307 commented Nov 5, 2022 • edited Loading

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Nov 7, 2022

Choose a reason for hiding this comment

eliasm307 Nov 7, 2022 • edited by JoshuaKGoldberg Loading

Choose a reason for hiding this comment

JoshuaKGoldberg Nov 7, 2022

Choose a reason for hiding this comment

JoshuaKGoldberg Nov 7, 2022

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

eliasm307 commented Sep 7, 2022 •

edited

Loading

nx-cloud bot commented Sep 7, 2022 •

edited

Loading

netlify bot commented Sep 7, 2022 •

edited

Loading

codecov bot commented Sep 7, 2022 •

edited

Loading

eliasm307 commented Nov 5, 2022 •

edited

Loading

eliasm307 Nov 7, 2022 •

edited by JoshuaKGoldberg

Loading