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

Fix: eslint.verify should not mutate config argument (fixes #8329) #8334

Merged
merged 1 commit into from Mar 28, 2017

Conversation

alberto
Copy link
Member

@alberto alberto commented Mar 26, 2017

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What parser (default, Babel-ESLint, etc.) are you using?
default

Please show your full configuration:
N/A

What did you do? Please include the actual source code causing the issue.

const eslint = require("eslint");

const config = {};

eslint.linter.verify("foo", config);

console.log(config);

What did you expect to happen?
I expected config to still be an empty object.

What actually happened? Please include the actual, raw output from ESLint.
A globals property was added to config, so the output was:

{ globals: {} }

What changes did you make? (Give an overview)
Avoid modifying the argument object. Fixes #8329

Is there anything you'd like reviewers to focus on?
No.

@alberto alberto added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features labels Mar 26, 2017
@eslintbot
Copy link

LGTM

@mention-bot
Copy link

@alberto, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mysticatea, @not-an-aardvark and @nzakas to be potential reviewers.

@ilyavolodin
Copy link
Member

@alberto Could you do perf run before and after this change?

@alberto
Copy link
Member Author

alberto commented Mar 26, 2017

@ilyavolodin no noticeable difference:

Before:

Loading:
  Load performance Run #1:  174.65021ms
  Load performance Run #2:  168.799739ms
  Load performance Run #3:  178.816711ms
  Load performance Run #4:  177.747853ms
  Load performance Run #5:  182.442309ms

  Load Performance median:  177.747853ms


Single File:
  CPU Speed is 2600 with multiplier 13000000
  Performance Run #1:  4207.118519ms
  Performance Run #2:  4098.461712ms
  Performance Run #3:  3699.8055409999997ms
  Performance Run #4:  3638.281188ms
  Performance Run #5:  3681.332236ms

  Performance budget ok:  3699.8055409999997ms (limit: 5000ms)


Multi Files (0 files):
  CPU Speed is 2600 with multiplier 39000000
  Performance Run #1:  12933.732048ms
  Performance Run #2:  12714.120701ms
  Performance Run #3:  12723.325213ms
  Performance Run #4:  12861.724682ms
  Performance Run #5:  12863.99631ms

  Performance budget ok:  12861.724682ms (limit: 15000ms)

After:

Loading:
  Load performance Run #1:  174.81977ms
  Load performance Run #2:  179.599493ms
  Load performance Run #3:  183.244189ms
  Load performance Run #4:  181.803644ms
  Load performance Run #5:  165.226383ms

  Load Performance median:  179.599493ms


Single File:
  CPU Speed is 2600 with multiplier 13000000
  Performance Run #1:  3713.787855ms
  Performance Run #2:  3659.128527ms
  Performance Run #3:  3659.855329ms
  Performance Run #4:  3786.704485ms
  Performance Run #5:  4434.773668ms

  Performance budget ok:  3713.787855ms (limit: 5000ms)


Multi Files (0 files):
  CPU Speed is 2600 with multiplier 39000000
  Performance Run #1:  12947.623801ms
  Performance Run #2:  12859.656538ms
  Performance Run #3:  12722.837927ms
  Performance Run #4:  12703.583254ms
  Performance Run #5:  13003.146405ms

  Performance budget ok:  12859.656538ms (limit: 15000ms)

@vitorbal vitorbal merged commit 3146167 into master Mar 28, 2017
@alberto alberto deleted the issue8329-verify-mutates-config branch March 29, 2017 09:00
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

linter.verify mutates its config argument
7 participants