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

Remove/relax typescript-eslint/prefer-destructuring #1830

Open
darkbasic opened this issue Oct 17, 2024 · 8 comments
Open

Remove/relax typescript-eslint/prefer-destructuring #1830

darkbasic opened this issue Oct 17, 2024 · 8 comments

Comments

@darkbasic
Copy link

image

How am I supposed to write this?

const [, , , , , , , , , row] = rows

This looks like madness to me.

@darkbasic
Copy link
Author

darkbasic commented Oct 17, 2024

Please note that this is an iterable so I cannot even use Array.at().

@darkbasic
Copy link
Author

I suspected this was part of your efforts against magic numbers, but even if I set the index as a const it still complains:

image

@darkbasic
Copy link
Author

darkbasic commented Oct 17, 2024

Zamiel suggested the following settings on Discord:

      /**
       * Object destructuring is enforced but array destructuring is not. This matches usage in the
       * general TypeScript ecosystem.
       */
      "@typescript-eslint/prefer-destructuring": [
        "warn",
        {
          VariableDeclarator: {
            array: false,
            object: true,
          },
          AssignmentExpression: {
            array: false,
            object: true,
          },
        },
        {
          // We disable this for renamed properties, since code like the following should be valid:
          // `const someSpecificMyEnum = MyEnum.Value1;`
          enforceForRenamedProperties: false,
        },
      ],

@mightyiam
Copy link
Owner

image

How am I supposed to write this?

const [, , , , , , , , , row] = rows

This looks like madness to me.

I agree. You are supposed to ad-hoc disable the rule where it's not helpful. Sorry for not making this clear earlier. See new readme section:
https://github.com/mightyiam/eslint-config-love/blob/01af4999a739cb4ec8c51e67c63f0bf3431f5246/README.md#disabling-rules

This is a quantitative consideration. Many rules are sometimes not useful and should be disabled, while in the majority of cases they are useful. Which do you think this is?

I suspected this was part of your efforts against magic numbers.

It was just the next rule to consider.

@darkbasic
Copy link
Author

This is a quantitative consideration. Many rules are sometimes not useful and should be disabled, while in the majority of cases they are useful. Which do you think this is?

I think that a rule can be enabled by default if it either:

  1. Doesn't provide any type safety benefit but it also never gets in your way.
  2. Most of the times it doesn't get in your way but it provides plenty enough type safety benefits to overcome the hurdle.

While object destructuring fits in the first category array destructuring doesn't fit in either.

If I have to add hundreds of overrides in my codebase the linting rules simply don't do their job well and should be split in two categories like vue does: recommended and strict.

I use linting rules both as a teaching tool for junior to mid-senior developers and as a weapon to prevent type unsafe code. If there are too many exceptions the linting doesn't fulfill either role because it confuses the developers and prompts them to always ignore them without thinking.

Please split your rules in two categories if this is not the path you want to follow: I've already seen too many people on Discord who maintain a set of overrides on top eslint-config-love and this is not the way you prompt adoption.

@mightyiam
Copy link
Owner

Yes, array restructuring is not a safety rule. It's a style rule. And it has a relatively high false positive rate, I suppose.

And you claim that it's not worth it. And that your students at some point are antagonized bt the number of errors. Okay. Perhaps it's appropriate to relax this rule in that project.

It's a trade-off. It's a quantitative question of where the line is drawn. How is the decision made? How much feedback does it take to change a decision? How many users are in agreement with this rule config? How many aren't?

Here's another idea: contribute an improvement to the rule so that it works better for everyone. For example, maybe it should only match with a reasonable number of skipped elements?

@darkbasic
Copy link
Author

It's a trade-off. It's a quantitative question of where the line is drawn. How is the decision made? How much feedback does it take to change a decision? How many users are in agreement with this rule config? How many aren't?

That's exactly the point I'm trying to make. We're at a point where we're starting to pile up overrides on top of overrides and I'm the only one who is qualified to upgrade eslint-config-love because on each and every upgrade I need to review all the changes and decide if they make sense or if they throw off the developers. If I didn't review the magic numbers rule I would have found my app full of shit like:

const ZERO = 0
for (const i = ZERO; i < something; i++) {

Senior colleagues are already starting to mock this library calling it "A TypeScript ESLint config that hates you" and someone even suggested to switch to one of the many "sanitized" forks that are starting to pop out.
Recently every time we inquire about some changes on the typescript-eslint Discord to gauge reactions they range from from "fuck no consider this officially nacked by the typescript-eslint team" to a more mild "I see no sane reason to enforce that".
It's difficult to find the perfect balance and takes many iterations, but we can't afford to throw untested, excessively opinionated and controversial stuff at users and call it a day. I suggest to make eslint-config-love configurable with different presets like eslint-plugin-vue: ...eslintPluginVue.configs['flat/recommended'].

That way we can still experiment with a more opinionated/bleeding edge config with potentially controversial changes and once they are baked in for a couple of months we can promote them to the recommended config where rules must meet one of the two criteria I've outline before.

Here's another idea: contribute an improvement to the rule so that it works better for everyone. For example, maybe it should only match with a reasonable number of skipped elements?

The point is that such discussion shouldn't happen at this stage. Rules that reach the recommended config should be past any major controversy and some real world testing must have happened. Requiring a const to initialize a loop should never ever reach a recommended config.

@mightyiam
Copy link
Owner

You want rules you don't agree with to never be released?
You believe that more testing would decrease the frequency of such occurrences?
You suggest some additional testing scheme?

I'd love to discuss that. Perhaps in #1841?

If anyone wishes to discuss this particular issue further, please do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

2 participants