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

Option to ignore array index in `no-magic-number`? #4370

Closed
mathieumg opened this Issue Nov 9, 2015 · 7 comments

Comments

Projects
None yet
5 participants
@mathieumg
Copy link
Contributor

mathieumg commented Nov 9, 2015

  1. The version of ESLint you are using.

    1.9.0

  2. The problem you want to solve.

    Accessing arrays using number literal indexes.

  3. Your take on the correct solution to problem.

    Would it be welcome to introduce an option in the no-magic-number rule such that number literals used to access array indexes are not considered magic numbers?

  4. What you did.

    const example = matcher.exec(/myRegEx/);
    
    if (example) {
      const a = res[1];
      const b = res[2];
      const c = res[3];
      const d = res[4];
    }

    ESLint complains about 3 (c) and 4(d), but it would also complain about 1 and 2 if they weren't default exceptions.

  5. What you would like to happen.

    With the proper option, the no-magic-number would not flag syntax that follows the array[#] format where # is a number literal.

  6. What actually happened.

    That currently is an error.

Thank you!

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Nov 9, 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 Nov 9, 2015

@lo1tuma

This comment has been minimized.

Copy link
Member

lo1tuma commented Nov 9, 2015

Can you explain why those numbers shouldn’t be considered to be magic?

The following example doesn't explain the context of the array index:

function extractItem(list) {
    return list[42];
}

BTW: sorry for accidentally closing the issue.

@lo1tuma lo1tuma closed this Nov 9, 2015

@lo1tuma lo1tuma reopened this Nov 9, 2015

@mathieumg

This comment has been minimized.

Copy link
Contributor Author

mathieumg commented Nov 9, 2015

They're sequential array indexes to access RegEx match groups. I don't see myself creating constants such as REGEX_MATCH_GROUP_X with X going from 0 to a high number "just to be safe". I agree however that the number in your counterexample is indeed magic, and I imagine there's no way with the current static analysis tooling/approach/complexity to figure out if we're dealing with just any array or one that represents a match group.

That being said, I'd like to use no-magic-numbers in a more relaxed way, so I wouldn't mind opting in to an option that disables all array index checks.

Thank you!

@IanVS IanVS added enhancement rule evaluating and removed triage labels Nov 10, 2015

@lo1tuma

This comment has been minimized.

Copy link
Member

lo1tuma commented Nov 10, 2015

Sounds reasonable to me.
On Nov 9, 2015 11:13 PM, "Mathieu M-Gosselin" notifications@github.com
wrote:

They're sequential array indexes to access RegEx match groups. I don't see
myself creating constants such as REGEX_MATCH_GROUP_X with X going from 0
to a high number "just to be safe". I agree however that the number in your
counterexample is indeed magic, and I imagine there's no way with the
current static analysis tooling/approach/complexity to figure out if we're
dealing with just any array or one that represents a match group.

That being said, I'd like to use no-magic-numbers in a more relaxed way,
so I wouldn't mind opting in to an option that disables all array index
checks.

Thank you!


Reply to this email directly or view it on GitHub
#4370 (comment).

@nzakas nzakas added accepted and removed evaluating labels Nov 11, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Nov 11, 2015

Let's do it. @mathieumg want to pull together a PR?

@mathieumg

This comment has been minimized.

Copy link
Contributor Author

mathieumg commented Nov 11, 2015

Cool! Sure, I'm just not sure when I'll get around to it in the next month as I'll be very busy with school and finals. :P Perhaps on a calm Friday night!

@nzakas nzakas added the help wanted label Nov 11, 2015

@nzakas nzakas closed this in 5ab564e Dec 27, 2015

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

Merge pull request #4819 from cschuller/issue-4370
New: 'ignoreArrayIndexes' option for 'no-magic-numbers' (fixes #4370)
@mathieumg

This comment has been minimized.

Copy link
Contributor Author

mathieumg commented Dec 28, 2015

I was thinking of looking into this in January, but thanks for handling it @cschuller ! I look forward to use the option. :)

@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.