fix(eslint-plugin): [naming-convention] cover case that requires quotes #4582

lonyele · 2022-02-22T00:05:57Z

PR Checklist

Addresses an existing issue: fixes [naming-convention] camelCase allows kebab-case in property #4534
That issue was marked as accepting prs
Steps in CONTRIBUTING.md were taken

Overview

Report errors if it needs quotes

nx-cloud · 2022-02-22T00:05:59Z

☁️ Nx Cloud Report

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

📂 See all runs for this branch

✅ Successfully ran 48 targets

Sent with 💌 from NxCloud.

typescript-eslint · 2022-02-22T00:06:01Z

Thanks for the PR, @lonyele!

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-02-22T00:06:07Z

✔️ Deploy Preview for typescript-eslint ready!

🔨 Explore the source changes: c0b4b29

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

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

codecov · 2022-02-22T00:15:18Z

Codecov Report

Merging #4582 (328c4a1) into main (fabfc2b) will increase coverage by 1.99%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##             main    #4582      +/-   ##
==========================================
+ Coverage   92.41%   94.41%   +1.99%     
==========================================
  Files         350      151     -199     
  Lines       12059     8229    -3830     
  Branches     3430     2623     -807     
==========================================
- Hits        11144     7769    -3375     
+ Misses        642      262     -380     
+ Partials      273      198      -75

Flag	Coverage Δ
unittest	`94.41% <100.00%> (+1.99%)`	⬆️

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

Impacted Files	Coverage Δ
...gin/src/rules/naming-convention-utils/validator.ts	`95.54% <100.00%> (+0.02%)`	⬆️
packages/type-utils/src/getTypeArguments.ts
...e-manager/src/definition/TSEnumMemberDefinition.ts
...s/scope-manager/src/scope/ClassStaticBlockScope.ts
...ges/typescript-estree/src/create-program/shared.ts
packages/typescript-estree/src/version-check.ts
packages/scope-manager/src/scope/ClassScope.ts
...ckages/scope-manager/src/referencer/TypeVisitor.ts
packages/scope-manager/src/lib/es2021.intl.ts
packages/scope-manager/src/lib/es2020.ts
... and 190 more

lonyele · 2022-02-22T00:16:59Z

hm... it took some time after the comment because I was trying to fix the playground but failed...

JoshuaKGoldberg

Source code looks great, thanks @lonyele!

Just requesting changes on a bit more test coverage please.

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

bradzacher

LGTM - thanks heaps for this!
@JoshuaKGoldberg feel free to merge if you're happy too 😄

JoshuaKGoldberg

Yup, looks great to me - thanks!

alfaproject · 2022-03-08T06:26:21Z

Oh man, this got really noisy in our projects. We work with a lot of external APIs and data sources and some of them use all kind of characters like #, @, even spaces

So, we basically quote those properties so that we bypass naming conventions but now that stopped working. Can we add a an option to ignore quoted properties? There's a reason they are quoted to begin with ):

lonyele · 2022-03-08T06:33:36Z

Oh god... I hear you. If maintainers agree with it. I'll make a PR that makes this an option

alfaproject · 2022-03-08T07:04:34Z

That would be awesome

fmal · 2022-03-08T07:16:49Z

@lonyele @alfaproject this becomes especially annoying with selectors when you use some css-in-js:

I don't want to add eslint-disable rule before selectors, nor do i want to specify some complex filter regex :(

alfaproject · 2022-03-08T07:33:42Z

Yeah that's another good use case to disable quoted properties

bradzacher · 2022-03-08T07:58:10Z

No need for an option - the rule is already designed to support this and it's already documented in the rule docs:
https://typescript-eslint.io/rules/naming-convention/#ignore-properties-that-require-quotes

alfaproject · 2022-03-08T08:34:11Z

Oh I was using filter to work around it. Didn't realise there was that modifier. Sorry for the noise, I'll give it a try

Edit: Actually, not quite sure how that works. Is this wrong? https://typescript-eslint.io/play/#ts=4.5.2&sourceType=module&code=PQKgUApgzgNglgOwC5gATtQIgAJIJ4AO0AxgE5wFIC008ywCAhgLaIDmVxA9ggG4TI4PTAC5UAbTQZp6TBFKkupTABopMjAG91GjJgBmcGEnmjUmrM0ZJiACzP7GMKBBVZSENhAAeZzOKoRADoAYmwAHXDxSIBdGMxUAF81XRkDJSskM3FMYhYIGABhRhdMGJTUvRcYCGIkJT8AEwhHAFdjTB0NZK6ZbUq9fQzrMwR2mAqBzGYuRrhDeShszA8AR1a4DygARVauEyXy3o1Matr65TEcrgAjACtzgBk4E1InAAVFIlJ8MuOMHoDfoDLCGYymMQWabWOwOJwuNwrTw+PwBYJhSLRcJxBKAkHpUiZZZ5ZgFYqlRHvEp5IolCCqLBQJgAawgAH08hSsABVd7vACiACU2YUAIIAZX5ZUmlUwNUYcwQbG5CGapCg3A8ficMC4AHdxewagB5UgAES4rRuNVU-2kpwK5waV0wtwedWerw+X3kv0R+CIny431+MTtSRl0jDIGAYDA3AQUCQqCGXFQAF5zDoAOQARmzYmzN0YpGzakSYCAA&rules=N4XyA&tsConfig=N4XyA

(the filters are my work-around for other quoted properties but I was hoping to get rid of them)

bradzacher · 2022-03-08T15:37:31Z

A filter will make the config higher precedence than others.
Hence it is matching before the requiresQuotes config.

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.13.0` -> `5.14.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/5.13.0/5.14.0) | | [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.13.0` -> `5.14.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/5.13.0/5.14.0) | --- ### Release Notes <details> <summary>typescript-eslint/typescript-eslint (@typescript-eslint/eslint-plugin)</summary> ### [`v5.14.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#5140-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5130v5140-2022-03-07) [Compare Source](typescript-eslint/typescript-eslint@v5.13.0...v5.14.0) ##### Bug Fixes - **eslint-plugin:** \[naming-convention] cover case that requires quotes ([#4582](typescript-eslint/typescript-eslint#4582)) ([3ea0947](typescript-eslint/typescript-eslint@3ea0947)) - **eslint-plugin:** \[no-misused-promises] factor thenable returning function overload signatures ([#4620](typescript-eslint/typescript-eslint#4620)) ([56a09e9](typescript-eslint/typescript-eslint@56a09e9)) - **eslint-plugin:** \[prefer-readonly-parameter-types] handle class sharp private field and member without throwing error ([#4343](typescript-eslint/typescript-eslint#4343)) ([a65713a](typescript-eslint/typescript-eslint@a65713a)) - **eslint-plugin:** \[return-await] correct autofixer in binary expression ([#4401](typescript-eslint/typescript-eslint#4401)) ([5fa2fad](typescript-eslint/typescript-eslint@5fa2fad)) ##### Features - **eslint-plugin:** \[no-misused-promises] add granular options within `checksVoidReturns` ([#4623](typescript-eslint/typescript-eslint#4623)) ([1085177](typescript-eslint/typescript-eslint@1085177)) </details> <details> <summary>typescript-eslint/typescript-eslint (@typescript-eslint/parser)</summary> ### [`v5.14.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#5140-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5130v5140-2022-03-07) [Compare Source](typescript-eslint/typescript-eslint@v5.13.0...v5.14.0) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) </details> --- ### Configuration 📅 **Schedule**: 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, click this checkbox. --- 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/1201 Reviewed-by: 6543 <6543@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

philip-firstorder · 2022-03-10T12:32:38Z

I think this should have been released to a major version, it currently broke our pipelines linting stages and we had no idea what caused this.
Anyone having a version lower than ^5.14.0 might suddenly get errors.

JoshuaKGoldberg · 2022-03-10T13:06:37Z

major version

Thus we see the great dislike from language, linter, and similar teams for the illusion of semantic versioning. Pretty much any logical change to a lint rule could be seen as a "breaking" change. Covering cases that previously weren't checked -both in the case of a bugfix and a lacking feature- breaks users who upgrade. If we released a major version every time one of these changes was released and wanted users to receive those updates at any reasonable cadence, we'd have to release a new major version every month and still be shipping dozens of breaking changes per version.

broke our pipelines linting stages

General good practice & our recommendation is to lock your dependencies to a fixed version, such as with a fixed range in a package.json and/or a lockfile. Use a tool such as dependabot or renovate to send PRs for specific package updates on a cadence.

Happy to move this discussion to a new issue if you think I'm wrong about either of those things and we should reevaluate our release strategy!

bradzacher · 2022-03-10T16:44:51Z

Definitely agree on the lock file - you should be using the same versions in dev as you do on your CI. If the two are running different versions then you're in for a bad time!

And strong agree on the versioning. Fixing a bug which uncovers cases that previously weren't caught is not what we define as a breaking change. For us breaking changes are pretty much only for cases where config needs to change.

It's similar to why TS doesn't follow semver - because it's impossible to change a lint rule without changingint results.

As per our contributing guidelines, we prefer to not have discussions on closed PRs because they're not easily searchable.
If you'd like to discuss this further - feel free to open an issue.

fix: cover case that requires quotes

116da9c

JoshuaKGoldberg requested changes Feb 23, 2022

View reviewed changes

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

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

bradzacher added the bug Something isn't working label Feb 23, 2022

lonyele added 2 commits February 27, 2022 14:05

test: cover more cases

db21512

chore: fix lint style

6da7dd0

bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed awaiting response Issues waiting for a reply from the OP or another party labels Mar 1, 2022

test: use noFormat for better testing case

9d12d86

bradzacher reviewed Mar 1, 2022

View reviewed changes

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

bradzacher added 2 commits February 28, 2022 20:32

nit add comment to test

5249603

Merge branch 'main' into fix/guard-quote

328c4a1

bradzacher approved these changes Mar 1, 2022

View reviewed changes

bradzacher added 1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready and removed awaiting response Issues waiting for a reply from the OP or another party labels Mar 1, 2022

bradzacher requested a review from JoshuaKGoldberg March 1, 2022 05:00

JoshuaKGoldberg approved these changes Mar 1, 2022

View reviewed changes

Merge branch 'main' into fix/guard-quote

c0b4b29

JoshuaKGoldberg enabled auto-merge (squash) March 1, 2022 13:40

JoshuaKGoldberg merged commit 3ea0947 into typescript-eslint:main Mar 1, 2022

typescript-eslint locked as resolved and limited conversation to collaborators Mar 10, 2022

lonyele deleted the fix/guard-quote branch March 11, 2022 05:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): [naming-convention] cover case that requires quotes #4582

fix(eslint-plugin): [naming-convention] cover case that requires quotes #4582

lonyele commented Feb 22, 2022

nx-cloud bot commented Feb 22, 2022 •

edited

typescript-eslint bot commented Feb 22, 2022

netlify bot commented Feb 22, 2022 •

edited

codecov bot commented Feb 22, 2022 •

edited

lonyele commented Feb 22, 2022 •

edited

JoshuaKGoldberg left a comment

bradzacher left a comment

JoshuaKGoldberg left a comment

alfaproject commented Mar 8, 2022

lonyele commented Mar 8, 2022

alfaproject commented Mar 8, 2022

fmal commented Mar 8, 2022

alfaproject commented Mar 8, 2022

bradzacher commented Mar 8, 2022

alfaproject commented Mar 8, 2022 •

edited

bradzacher commented Mar 8, 2022

philip-firstorder commented Mar 10, 2022

JoshuaKGoldberg commented Mar 10, 2022

bradzacher commented Mar 10, 2022

fix(eslint-plugin): [naming-convention] cover case that requires quotes #4582

fix(eslint-plugin): [naming-convention] cover case that requires quotes #4582

Conversation

lonyele commented Feb 22, 2022

PR Checklist

Overview

nx-cloud bot commented Feb 22, 2022 • edited

☁️ Nx Cloud Report

typescript-eslint bot commented Feb 22, 2022

netlify bot commented Feb 22, 2022 • edited

codecov bot commented Feb 22, 2022 • edited

Codecov Report

lonyele commented Feb 22, 2022 • edited

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

bradzacher left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

alfaproject commented Mar 8, 2022

lonyele commented Mar 8, 2022

alfaproject commented Mar 8, 2022

fmal commented Mar 8, 2022

alfaproject commented Mar 8, 2022

bradzacher commented Mar 8, 2022

alfaproject commented Mar 8, 2022 • edited

bradzacher commented Mar 8, 2022

philip-firstorder commented Mar 10, 2022

JoshuaKGoldberg commented Mar 10, 2022

bradzacher commented Mar 10, 2022

nx-cloud bot commented Feb 22, 2022 •

edited

netlify bot commented Feb 22, 2022 •

edited

codecov bot commented Feb 22, 2022 •

edited

lonyele commented Feb 22, 2022 •

edited

alfaproject commented Mar 8, 2022 •

edited