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

Why is no-magic-number default "ignore" option set to [0, 1, 2]? #4193

Closed
alberto opened this Issue Oct 19, 2015 · 11 comments

Comments

Projects
None yet
5 participants
@alberto
Copy link
Member

alberto commented Oct 19, 2015

It caught me by surprise that no-magic-number ignores numbers [0, 1, 2] by default. That's pretty magic if you ask me :).

If there is a rationale behind that (I can't think of any), it should be documented in the rule. Otherwise it should not ignore any number by default.

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Oct 19, 2015

Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Oct 19, 2015

@alberto alberto changed the title Why is no-magic-number default ignored option set to [0, 1, 2] Why is no-magic-number default "ignore" option set to [0, 1, 2]? Oct 19, 2015

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Oct 19, 2015

Yeah maybe it shouldn't have a default and just require an array

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 19, 2015

It is documented: http://eslint.org/docs/rules/no-magic-numbers#ignore

The rationale is that these are the numbers people use most frequently, such as:

if (items.length === 0) {}
if (items.length > = 1) {}

Etc.

Honestly, I didn't catch this on code review so I was a bit surprised too, but I think it makes sense.

If we want to make a change, it will have to wait for 2.0.0.

@alberto

This comment has been minimized.

Copy link
Member Author

alberto commented Oct 19, 2015

I meant the reason should be documented.
IMO 0 could get a pass, 1 is dubious, but 2?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 19, 2015

Fair point. @kombucha what's the rationale for each of the default allowed magic numbers?

@nzakas nzakas added bug documentation accepted and removed triage labels Oct 19, 2015

@kombucha

This comment has been minimized.

Copy link
Contributor

kombucha commented Oct 19, 2015

I mimicked the default behaviour of buddy.js.
I see know that it was just [0, 1], [0, 1, 2] was just an example in the docs.
And when I look at the code of buddy.js, the defaults seems to actually be an empty array...

@nzakas nzakas added rule breaking and removed documentation labels Oct 19, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 19, 2015

Okay, let's plan on changing this for 2.0.0.

@kombucha

This comment has been minimized.

Copy link
Contributor

kombucha commented Oct 19, 2015

I guess an empty array as a default might make more sense, and just let people decide what make sense in their project.
Or 0 and -1, since they're so pervasive when manipulating arrays.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 19, 2015

I'd go with an empty array as the default - let people add what they want and then there's no controversy.

@nzakas nzakas added this to the v2.0.0 milestone Oct 19, 2015

@alberto

This comment has been minimized.

Copy link
Member Author

alberto commented Oct 19, 2015

@nzakas +1 on that

Can this be tackled now, or do you prefer to wait?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 19, 2015

Best to wait until we're ready for 2.0.0. I hate having PRs sitting around.

@alberto alberto closed this in a805060 Dec 1, 2015

nzakas added a commit that referenced this issue Dec 1, 2015

Merge pull request #4502 from eslint/issue4193
Breaking: Default no-magic-numbers to none. (fixes #4193)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.