Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
fix: missing placeholders in violation messages forno-unnecessary-type-constraint andno-unsafe-argument (and enableeslint-plugin/recommended rules internally)#5453
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…pe-constraint and no-unsafe-argument (and enable eslint-plugin/recommended rules internally)
Thanks for the PR,@bmish! 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 onhttps://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitorsper day. |
nx-cloudbot commentedAug 9, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
netlifybot commentedAug 9, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify site settings. |
| @@ -194,7 +195,7 @@ module.exports = { | |||
| '@typescript-eslint/no-unsafe-call': 'off', | |||
| '@typescript-eslint/no-unsafe-member-access': 'off', | |||
| '@typescript-eslint/no-unsafe-return': 'off', | |||
| 'eslint-plugin/no-identical-tests': 'error', | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This rule is now included in therecommended config.
| @@ -9,7 +9,7 @@ export default util.createRule({ | |||
| description: 'Disallow duplicate enum member values', | |||
| recommended: 'strict', | |||
| }, | |||
| hasSuggestions:true, | |||
| hasSuggestions:false, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Many of these rules indicated they had suggestions but do not actually provide suggestions.
Caught by theeslint-plugin/require-meta-has-suggestions rule.
| @@ -15,10 +15,9 @@ export default util.createRule<Options, MessageIds>({ | |||
| docs: { | |||
| description: 'Disallow the declaration of empty interfaces', | |||
| recommended: 'error', | |||
| suggestion: true, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thismeta.docs.suggestion property was replaced bymeta.hasSuggestions (more info ineslint/eslint#14312). It's confusing to leave the old property here especially when it's incorrectly specified in many of the rules.
| @@ -89,6 +88,9 @@ export default util.createRule({ | |||
| suggest: [ | |||
| { | |||
| messageId: 'removeUnnecessaryConstraint', | |||
| data: { | |||
| constraint, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is an actual bug. This data was missing for the suggestion message and would thus show as{{constraint}} to the user instead of the actual value. I updated the tests to test this data.
I can extract this bug fix into a separate PR if desired.
Caught by theeslint-plugin/no-missing-placeholders andeslint-plugin/no-unused-placeholders rules.
| @@ -142,7 +142,7 @@ export default util.createRule<[], MessageIds>({ | |||
| unsafeArgument: | |||
| 'Unsafe argument of type `{{sender}}` assigned to a parameter of type `{{receiver}}`.', | |||
| unsafeTupleSpread: | |||
| 'Unsafe spread of a tuple type. The{{index}} elementis of type `{{sender}}` and is assigned to a parameter of type `{{reciever}}`.', | |||
| 'Unsafe spread of a tuple type. Theargumentis of type `{{sender}}` and is assigned to a parameter of type `{{receiver}}`.', | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is an actual bug. The{{index}} placeholder was never provided for this message. It also doesn't seem necessary to me as the violation is already reported on the specific argument that it refers to.
I can extract this bug fix into a separate PR if desired.
Caught by theeslint-plugin/no-missing-placeholders andeslint-plugin/no-unused-placeholders rules.
| @@ -142,7 +142,7 @@ export default util.createRule<[], MessageIds>({ | |||
| unsafeArgument: | |||
| 'Unsafe argument of type `{{sender}}` assigned to a parameter of type `{{receiver}}`.', | |||
| unsafeTupleSpread: | |||
| 'Unsafe spread of a tuple type. The{{index}} elementis of type `{{sender}}` and is assigned to a parameter of type `{{reciever}}`.', | |||
| 'Unsafe spread of a tuple type. Theargumentis of type `{{sender}}` and is assigned to a parameter of type `{{receiver}}`.', | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is an actual bug. There is a typo in thereceiver placeholder name.
I can extract this bug fix into a separate PR if desired.
Caught by theeslint-plugin/no-missing-placeholders andeslint-plugin/no-unused-placeholders rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wow, I had no idea eslint-plugin-eslint-plugin (😂 what a name!) was so powerful! Fantastic stuff this PR brings in. Thanks for sending it over!
LGTM, just a few notes on filing & linking issues. I'm down to help fill those requests in if it'd be helpful!
packages/eslint-plugin/tests/rules/consistent-indexed-object-style.test.tsShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
.eslintrc.js Outdated
| @@ -194,7 +195,7 @@ module.exports = { | |||
| '@typescript-eslint/no-unsafe-call': 'off', | |||
| '@typescript-eslint/no-unsafe-member-access': 'off', | |||
| '@typescript-eslint/no-unsafe-return': 'off', | |||
| 'eslint-plugin/no-identical-tests': 'error', | |||
| 'eslint-plugin/consistent-output': 'off', | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Out of curiosity, why is this still enabled inplugin:eslint-plugin/recommended? Is it just a legacy thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I disabledeslint-plugin/consistent-output for two reasons:
- There are 100 violations so we could follow-up to enable it in a separate PR if we want to.
- I think it's less useful now that ESLint v7validates that test cases provide output when they produce an autofix, and it's tedious to include
output: nullwhen not necessary. It might beremoved asrecommendedeventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Heh, yeah, now that ESLint is already at 8.21.0, it feels like removing from recommended is the right step 😄
Could you just add a todo comment around this line with a link to an issue? Either one on this repo if you think we should enable it, or one on the eslint-plugin repo if you think it should be removed fromrecommended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Filed and added a link to this issue:eslint-community/eslint-plugin-eslint-plugin#284
| @@ -1,3 +1,7 @@ | |||
| /* eslint-disable eslint-comments/no-use */ | |||
| /* eslint eslint-plugin/no-unused-message-ids:"off" -- disabled because the rule doesn't understand our helper function checkCall() */ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We generally like filing issues when a bug is found, both to get a link to a full discussion & to help drive to fixing it. Do you have the bandwidth to file one so we can link it here?
(here and the other disable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I went ahead and filed these bugs to capture the two @typescript-eslint/eslint-plugin bugs I'm fixing:
- Bug:
no-unnecessary-type-constraintmissing placeholder in suggestion message #5460 - Bug:
no-unsafe-argumentmissing placeholders in violation message #5461
I also filed and added links for the remaining places a rule had to be disabled:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Awesome, thanks! ✨
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is fantastic work, thanks so much for sending it in! 👏
Just requesting changes on adding some comments around disables, to help us track removing them over time. Otherwise I think this is good to ship!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Glorious, thanks for introducingeslint-plugin-eslint-plugin to the repo (and me)! Let's get these fixes in 💪
This PR contains the following updates:| Package | Type | Update | Change ||---|---|---|---|| [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | patch | [`5.33.0` -> `5.33.1`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/5.33.0/5.33.1) || [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | patch | [`5.33.0` -> `5.33.1`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/5.33.0/5.33.1) |---### Release Notes<details><summary>typescript-eslint/typescript-eslint (@​typescript-eslint/eslint-plugin)</summary>### [`v5.33.1`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#​5331-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5330v5331-2022-08-15)[Compare Source](typescript-eslint/typescript-eslint@v5.33.0...v5.33.1)##### Bug Fixes- missing placeholders in violation messages for `no-unnecessary-type-constraint` and `no-unsafe-argument` (and enable `eslint-plugin/recommended` rules internally) ([#​5453](typescript-eslint/typescript-eslint#5453)) ([d023910](typescript-eslint/typescript-eslint@d023910))</details><details><summary>typescript-eslint/typescript-eslint (@​typescript-eslint/parser)</summary>### [`v5.33.1`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#​5331-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5330v5331-2022-08-15)[Compare Source](typescript-eslint/typescript-eslint@v5.33.0...v5.33.1)**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.--- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.---This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xNTkuMSIsInVwZGF0ZWRJblZlciI6IjMyLjE1OS4xIn0=-->Co-authored-by: cabr2-bot <cabr2.help@gmail.com>Reviewed-on:https://codeberg.org/Calciumdibromid/CaBr2/pulls/1509Reviewed-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>
This PR contains the following updates:| Package | Type | Update | Change ||---|---|---|---|| [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.33.1` -> `5.35.1`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/5.33.1/5.35.1) || [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.33.1` -> `5.35.1`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/5.33.1/5.35.1) |---### Release Notes<details><summary>typescript-eslint/typescript-eslint (@​typescript-eslint/eslint-plugin)</summary>### [`v5.35.1`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#​5351-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5350v5351-2022-08-24)[Compare Source](typescript-eslint/typescript-eslint@v5.35.0...v5.35.1)##### Bug Fixes- **eslint-plugin:** correct rule schemas to pass ajv validation ([#​5531](typescript-eslint/typescript-eslint#5531)) ([dbf8b56](typescript-eslint/typescript-eslint@dbf8b56))### [`v5.35.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#​5350-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5340v5350-2022-08-24)[Compare Source](typescript-eslint/typescript-eslint@v5.34.0...v5.35.0)##### Features- **eslint-plugin:** \[explicit-member-accessibility] suggest adding explicit accessibility specifiers ([#​5492](typescript-eslint/typescript-eslint#5492)) ([0edb94a](typescript-eslint/typescript-eslint@0edb94a))### [`v5.34.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#​5340-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5331v5340-2022-08-22)[Compare Source](typescript-eslint/typescript-eslint@v5.33.1...v5.34.0)##### Bug Fixes- **eslint-plugin:** \[no-useless-constructor] handle parameter decorator ([#​5450](typescript-eslint/typescript-eslint#5450)) ([864dbcf](typescript-eslint/typescript-eslint@864dbcf))##### Features- **eslint-plugin:** \[prefer-optional-chain] support suggesting `!foo || !foo.bar` as a valid match for the rule ([#​5266](typescript-eslint/typescript-eslint#5266)) ([aca935c](typescript-eslint/typescript-eslint@aca935c))#### [5.33.1](typescript-eslint/typescript-eslint@v5.33.0...v5.33.1) (2022-08-15)##### Bug Fixes- missing placeholders in violation messages for `no-unnecessary-type-constraint` and `no-unsafe-argument` (and enable `eslint-plugin/recommended` rules internally) ([#​5453](typescript-eslint/typescript-eslint#5453)) ([d023910](typescript-eslint/typescript-eslint@d023910))</details><details><summary>typescript-eslint/typescript-eslint (@​typescript-eslint/parser)</summary>### [`v5.35.1`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#​5351-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5350v5351-2022-08-24)[Compare Source](typescript-eslint/typescript-eslint@v5.35.0...v5.35.1)**Note:** Version bump only for package [@​typescript-eslint/parser](https://github.com/typescript-eslint/parser)### [`v5.35.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#​5350-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5340v5350-2022-08-24)[Compare Source](typescript-eslint/typescript-eslint@v5.34.0...v5.35.0)**Note:** Version bump only for package [@​typescript-eslint/parser](https://github.com/typescript-eslint/parser)### [`v5.34.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#​5340-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5331v5340-2022-08-22)[Compare Source](typescript-eslint/typescript-eslint@v5.33.1...v5.34.0)**Note:** Version bump only for package [@​typescript-eslint/parser](https://github.com/typescript-eslint/parser)#### [5.33.1](typescript-eslint/typescript-eslint@v5.33.0...v5.33.1) (2022-08-15)**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.--- - [x] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.---This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xNjkuMSIsInVwZGF0ZWRJblZlciI6IjMyLjE3My4wIn0=-->Co-authored-by: cabr2-bot <cabr2.help@gmail.com>Co-authored-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>Reviewed-on:https://codeberg.org/Calciumdibromid/CaBr2/pulls/1519Reviewed-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>
Uh oh!
There was an error while loading.Please reload this page.
PR Checklist
no-unnecessary-type-constraintmissing placeholder in suggestion message #5460fixesBug:no-unsafe-argumentmissing placeholders in violation message #5461Overview
Fixes bugs caught by theeslint-plugin/no-missing-placeholders andeslint-plugin/no-unused-placeholders rules:
no-unnecessary-type-constraintmissing placeholder in suggestion message #5460no-unsafe-argumentmissing placeholders in violation message #5461Enables the
recommendedconfig fromeslint-plugin-eslint-plugin which catches mistakes and enforces best practices in eslint plugins. Note that I have been actively making fixes to eslint-plugin-eslint-plugin to eliminate any false positives with its rules.If desired, I can extract these bug fixes into separate PRs to go in ahead of the rest of the eslint-plugin-eslint-plugin additions.