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

prefer-destructuring false Positive when casting result #13678

Closed
sk- opened this issue Sep 9, 2020 · 6 comments
Closed

prefer-destructuring false Positive when casting result #13678

sk- opened this issue Sep 9, 2020 · 6 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules

Comments

@sk-
Copy link

sk- commented Sep 9, 2020

Tell us about your environment

Environment Info:

Node version: v14.6.0
npm version: v6.14.7
Local ESLint version: v7.8.1 (Currently used)
Global ESLint version: Not found

What parser (default, @babel/eslint-parser, @typescript-eslint/parser, etc.) are you using?
default
Please show your full configuration:

Configuration
"prefer-destructuring": ["error", {"VariableDeclarator": {"object": true}}],

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

const transactionData =
      /** @type {Transaction} */
      (data.transactionData);

What did you expect to happen?
Not to complain about destructuring

What actually happened? Please include the actual, raw output from ESLint.

error  Use object destructuring  prefer-destructuring

Are you willing to submit a pull request to fix this bug?
Depending on whether I'm pointed in the right direction.

@sk- sk- added bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Sep 9, 2020
@mdjermanovic mdjermanovic added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules autofix This change is related to ESLint's autofixing capabilities and removed triage An ESLint team member will look at this issue soon labels Sep 9, 2020
@mdjermanovic
Copy link
Member

Hi @sk-, thanks for the bug report!

I'm not sure if this is a bug, but I think it's reasonable to support this use case, either by default or behind an option.

Maybe we could check if there are any comments between the = and the right side, and not report an error in such a case.

There's also a bug that should be certainly fixed: autofix removes that comment, which should never happen. At least the rule shouldn't auto-fix this code.

@mdjermanovic
Copy link
Member

Would the following be equivalent to the original code?

const { transactionData } =
      /** @type {{ transactionData: Transaction }} */
      (data);

@sk-
Copy link
Author

sk- commented Sep 10, 2020

@mdjermanovic yes, that works, but in that case we lose all the benefits of destructuring, as transactionData is now also repeated. Compare:

const transactionData =
      /** @type {Transaction} */
      (data.transactionData);

vs

const { transactionData } =
      /** @type {{ transactionData: Transaction }} */
      (data);

The second option is even more verbose, as there are 2 extra {} pairs, plus the extra whitespace.

@mdjermanovic mdjermanovic added enhancement This change enhances an existing feature of ESLint and removed autofix This change is related to ESLint's autofixing capabilities bug ESLint is working incorrectly labels Sep 10, 2020
@mdjermanovic
Copy link
Member

Thanks for clarifying! Then, I think this isn't a bug, but I'll support a configuration option to not report an error if there are any comments inside the declaration/assignment. Let's see if this enhancement will reach a consensus from the team.

I'm working on the autofix bug separately.

@anikethsaha
Copy link
Member

Is it a stylistic rule? it is in the category of es6 but It does works for stylistic preference I suppose.
If it is a stylistic rule then supporting this check will fall under new policies.

If not, IMO, it should be in jsdoc-plugins right ? cause js doesn't understand that typecasting using comments.

WDYT ?

@eslint-deprecated
Copy link

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Apr 12, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

3 participants