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

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

Merged
merged 14 commits into from Aug 20, 2020

Conversation

@Moeriki
Copy link
Contributor

@Moeriki Moeriki commented Nov 27, 2019

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?

@eslint eslint bot added the triage label Nov 27, 2019
@Moeriki
Copy link
Contributor Author

@Moeriki Moeriki commented Nov 27, 2019

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?

@Moeriki Moeriki force-pushed the Moeriki:no-magic-number-ignore-default-values branch from 40a92dc to ee1ef95 Nov 29, 2019
@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Dec 21, 2019

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
Copy link
Member

@mdjermanovic mdjermanovic commented Feb 7, 2020

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
Copy link
Contributor Author

@Moeriki Moeriki commented Feb 7, 2020

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
Copy link
Member

@mdjermanovic mdjermanovic commented Feb 9, 2020

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
Copy link
Contributor Author

@Moeriki Moeriki commented Feb 10, 2020

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
Copy link
Contributor Author

@Moeriki Moeriki commented Apr 27, 2020

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

Besides rebase and resolve conflicts 😁

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Jun 12, 2020

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

@nzakas nzakas self-assigned this Jun 19, 2020
@nzakas
Copy link
Member

@nzakas nzakas commented Jun 19, 2020

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

@btmills
Copy link
Member

@btmills btmills commented Jun 19, 2020

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

@btmills btmills added accepted and removed evaluating labels Jun 19, 2020
@Moeriki Moeriki force-pushed the Moeriki:no-magic-number-ignore-default-values branch from ee1ef95 to d07b0b0 Jun 20, 2020
Copy link
Member

@kaicataldo kaicataldo left a comment

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

const [ el = 123] = arr;
docs/rules/no-magic-numbers.md Show resolved Hide resolved
lib/rules/no-magic-numbers.js Outdated Show resolved Hide resolved
tests/lib/rules/no-magic-numbers.js Show resolved Hide resolved
lib/rules/no-magic-numbers.js Outdated Show resolved Hide resolved
tests/lib/rules/no-magic-numbers.js Show resolved Hide resolved
@Moeriki Moeriki force-pushed the Moeriki:no-magic-number-ignore-default-values branch from 7276334 to 4cda76d Jun 30, 2020
@mdjermanovic mdjermanovic changed the title Fix: Add no-magic-numbers 'ignoreDefaultValues' option Update: Add no-magic-numbers 'ignoreDefaultValues' option Jun 30, 2020
Copy link
Member

@btmills btmills left a comment

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

lib/rules/no-magic-numbers.js Outdated Show resolved Hide resolved
tests/lib/rules/no-magic-numbers.js Show resolved Hide resolved
docs/rules/no-magic-numbers.md Outdated Show resolved Hide resolved
lib/rules/no-magic-numbers.js Outdated Show resolved Hide resolved
lib/rules/no-magic-numbers.js Outdated Show resolved Hide resolved
lib/rules/no-magic-numbers.js Outdated Show resolved Hide resolved
@btmills
btmills approved these changes Jul 3, 2020
Copy link
Member

@btmills btmills left a comment

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

Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks!

@btmills
Copy link
Member

@btmills btmills commented Jul 13, 2020

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

@btmills btmills requested a review from kaicataldo Jul 20, 2020
@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Jul 31, 2020

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

…gnore-default-values

* upstream/master: (66 commits)
  Sponsors: Sync README with website
  Sponsors: Sync README with website
  Sponsors: Sync README with website
  Sponsors: Sync README with website
  Sponsors: Sync README with website
  Chore: remove leche (fixes #13287) (#13533)
  Sponsors: Sync README with website
  Sponsors: Sync README with website
  7.6.0
  Build: changelog update for 7.6.0
  Update: require `meta` for fixable rules in RuleTester (refs #13349) (#13489)
  Docs: fix broken links in developer guide (#13518)
  Fix: Do not output `undefined` as line and column when it's unavailable (#13519)
  Sponsors: Sync README with website
  Sponsors: Sync README with website
  Fix: Update the chatroom link to go directly to help channel (#13536)
  Sponsors: Sync README with website
  Update: Change no-duplicate-case to comparing tokens (fixes #13485) (#13494)
  Docs: add ECMAScript 2020 to README (#13510)
  7.5.0
  ...
@jsf-clabot
Copy link

@jsf-clabot jsf-clabot commented Aug 9, 2020

CLA assistant check
All committers have signed the CLA.

@Moeriki
Copy link
Contributor Author

@Moeriki Moeriki commented Aug 9, 2020

Merged master back in.

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

@btmills btmills dismissed kaicataldo’s stale review Aug 20, 2020

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

@btmills btmills merged commit 66442a9 into eslint:master Aug 20, 2020
12 checks passed
12 checks passed
Verify Files
Details
Test (ubuntu-latest, 14.x)
Details
Test (ubuntu-latest, 13.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 10.12.0)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message PR title follows commit message guidelines
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@btmills
Copy link
Member

@btmills btmills commented Aug 20, 2020

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

@Moeriki Moeriki deleted the Moeriki:no-magic-number-ignore-default-values branch Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.