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.

Sign up for GitHub

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

Jump to bottom

Chore: Replace some function application with spread operators #10645

Merged
merged 1 commit into from
Jul 22, 2018
Merged

Chore: Replace some function application with spread operators #10645

merged 1 commit into from
Jul 22, 2018

Conversation

platinumazure
Copy link
Member

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

[ ] Documentation update
[ ] 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
[x] Other, please explain: Just replacing some calls to Function.prototype.apply by using spread operators (or, in a few cases, Function.prototype.call.

What changes did you make? (Give an overview)

Replaced some function application with spread operators.

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

Not really.

@platinumazure platinumazure added accepted There is consensus among the team that this change meets the criteria for inclusion chore This change is not user-facing labels Jul 22, 2018
@platinumazure
Copy link
Member Author

I ran some performance tests just to see if anything showed up. Seems negligible.

Before:


> eslint@5.2.0 perf C:\Users\Kevin\Documents\GitHub\eslint
> node Makefile.js perf


Loading:
  Load performance Run #1:  481.270948ms
  Load performance Run #2:  485.966138ms
  Load performance Run #3:  494.304693ms
  Load performance Run #4:  481.03489ms
  Load performance Run #5:  486.99524ms

  Load Performance median:  485.966138ms


Single File:
  CPU Speed is 2208 with multiplier 13000000
  Performance Run #1:  12369.628925ms
  Performance Run #2:  12088.161432ms
  Performance Run #3:  12014.560007ms
  Performance Run #4:  12105.00828ms
  Performance Run #5:  11961.682069ms

  Performance budget exceeded: 12088.161432ms (limit: 5887.68115942029ms)


Multi Files (0 files):
  CPU Speed is 2208 with multiplier 39000000
  Performance Run #1:  29699.744325ms
  Performance Run #2:  29950.80021ms
  Performance Run #3:  29290.550859ms
  Performance Run #4:  29211.821083ms
  Performance Run #5:  30003.076175ms

  Performance budget exceeded: 29699.744325ms (limit: 17663.043478260868ms)

After:

> eslint@5.2.0 perf C:\Users\Kevin\Documents\GitHub\eslint
> node Makefile.js perf


Loading:
  Load performance Run #1:  484.968109ms
  Load performance Run #2:  481.33773ms
  Load performance Run #3:  490.3993ms
  Load performance Run #4:  480.28544ms
  Load performance Run #5:  486.001849ms

  Load Performance median:  484.968109ms


Single File:
  CPU Speed is 2208 with multiplier 13000000
  Performance Run #1:  12330.415922ms
  Performance Run #2:  11991.310372ms
  Performance Run #3:  12168.240774ms
  Performance Run #4:  11979.039527ms
  Performance Run #5:  12050.532662ms

  Performance budget exceeded: 12050.532662ms (limit: 5887.68115942029ms)


Multi Files (0 files):
  CPU Speed is 2208 with multiplier 39000000
  Performance Run #1:  29812.813363ms
  Performance Run #2:  29327.754355ms
  Performance Run #3:  29198.634759ms
  Performance Run #4:  29909.118566ms
  Performance Run #5:  30264.074325ms

  Performance budget exceeded: 29812.813363ms (limit: 17663.043478260868ms)

aladdin-add
aladdin-add reviewed Jul 22, 2018
View reviewed changes
tests/lib/rules/no-invalid-this.js
@@ -83,7 +83,7 @@ function extractPatterns(patterns, type) {
}));

// Flatten.
return Array.prototype.concat.apply([], patternsList);
return [].concat(...patternsList);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possibly be return [...patternsList)]?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I tried this and it doesn't work. patternList is an array of arrays that needs to be flattened. [...patternList] just shallow-copies the array of arrays. [].concat(...patternsList) flattens the array. Thanks for the suggestion though, it was worth a try!

not-an-aardvark
not-an-aardvark approved these changes Jul 22, 2018
View reviewed changes
Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@not-an-aardvark not-an-aardvark merged commit 0cb5e3e into master Jul 22, 2018
@not-an-aardvark not-an-aardvark deleted the remove-function-apply branch July 22, 2018 22:34
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jan 19, 2019
@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 Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@not-an-aardvark not-an-aardvark not-an-aardvark approved these changes

@aladdin-add aladdin-add aladdin-add approved these changes

Assignees
No one assigned
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 chore This change is not user-facing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@platinumazure @not-an-aardvark @aladdin-add