Update: Add no-magic-numbers 'ignoreDefaultValues' option

[moeriki]

This was briefly discussed in #10751 and #11271. As far as I could tell people thought it was a good idea.

What is the purpose of this pull request?

[X] Changes an existing rule

What rule do you want to change?

no-magic-numbers

Does this change cause the rule to produce more or fewer warnings?

Fewer.

How will the change be implemented?

I added an option called ignoreDefaultValues.

Please provide some example code that this change will affect:

const { WEB = 3000 } = process.env;
const mapPromises = ({ concurrency = 2 }) => {};

What does the rule currently do for this code?

Error: No magic number: 3000
Error: No magic number: 2

What will the rule do after it's changed?

∅

[moeriki]

I'd consider ignoring nullish coalescing assignment through the same option.

const port = process.env.PORT ?? 3000;

This can be done with the following additional check.

parent.type === 'LogicalExpression' && parent.operator === '??'

What do you think?

[kaicataldo]

I'd consider ignoring nullish coalescing assignment through the same option.

I think this makes sense, though I don't know if it makes sense to put it behind the ignoreDefaultValues flag.

I support this change. We'll need a champion and two more supporters from the team before we can mark this as accepted.

[mdjermanovic]

Should the option allow all default values regardless of the context? For example, the following are also default assignments (AssignmentPattern):

let WEB;
({ WEB = 3000 } = process.env);

const [foo = 89] = bar;

function doSomething(foo = 33) {}

for (const { value = 42 } of values) {}

[moeriki]

Should the option allow all default values regardless of the context?

I think yes.

For me the purpose is to allow numbers where the variable is providing the necessary description.

[mdjermanovic]

Thanks for the clarification!

If the proposal gets accepted we should certainly add more examples in the documentation and more tests to specify the intention. Someone might expect that the option applies only to declarations, while it also allows magic numbers in assignments, e.g.,:

[foo = 35, bar = 20] = getSomething();

[moeriki]

we should certainly add more examples in the documentation and more tests to specify the intention.

Agreed. I'm awaiting maintainer approval to update the PR.

[moeriki]

Hi all! I would still love this option. Is there anything I can do to move this forward?

Besides rebase and resolve conflicts 😁

[kaicataldo]

@eslint/eslint-team It looks like we need a champion and one more 👍 to accept this.

[nzakas]

I will champion this. @moeriki if you're still up to update this PR, we can move forward. Sorry for the delay.

[btmills]

This makes a lot of sense. Added my 👍 and marking as accepted!

[kaicataldo]

Code LGTM, but it looks like we're missing tests for array destructuring with default values. e.g.

const [ el = 123] = arr;

[btmills]

Two minor alterations, then LGTM - thanks for working on this @moeriki!

[btmills]

Other than the two JSDoc comment updates requested by @mdjermanovic, LGTM - thank you!

[mdjermanovic]

LGTM, thanks!

[btmills]

@kaicataldo if you're happy with the array destructuring default tests in 7a942b6, then this can be merged!

[kaicataldo]

@moeriki Do you mind fixing up the merge conflict? Otherwise LGTM.

[jsf-clabot]

All committers have signed the CLA.

[moeriki]

Merged master back in.

I mistakenly removed my committer email from my GitHub account. I did add it again. The CLA should be fine.

The most recent review was "Otherwise LGTM" once the merge conflict was resolved, which it has been.

[btmills]

Thanks @moeriki! I'm looking forward to having this in the next release.