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
Chore: rewrite parseListConfig for a small perf gain. #9300
Conversation
LGTM |
1 similar comment
LGTM |
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
I'm skeptical that this actually improves performance by any noticeable amount, because parsing comment configs is a very small portion of an overall linting run. Results from microbenchmarks often don't convert into real end-to-end performance boosts for more complex applications.
That said, I'm fine with this change anyway as a readability improvement over .replace
, provided rename one of the variables to be more descriptive.
lib/linter.js
Outdated
@@ -145,17 +145,15 @@ function parseJsonConfig(string, location) { | |||
*/ | |||
function parseListConfig(string) { | |||
const items = {}; | |||
const reg = /[\w-/]+/g; | |||
const rs = string.match(reg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give this variable a more descriptive name? The name rs
doesn't communicate what the variable is doing very well.
Maybe something like elementMatches
would be better.
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Every drop counts. 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one question from me. Thanks for contributing!
lib/linter.js
Outdated
@@ -145,17 +145,15 @@ function parseJsonConfig(string, location) { | |||
*/ | |||
function parseListConfig(string) { | |||
const items = {}; | |||
const reg = /[\w-/]+/g; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this hyphen be escaped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, before /
, it's no need. I was confused.
added a \
in case.
It has special meaning in [a-z] but maybe not when it is preceded by \w. I
would feel better seeing it after [ or before ]. If there are negative
tests for this logic then I'm okay with no change. Thanks!
…On Sep 14, 2017 12:23 PM, "薛定谔的猫" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/linter.js
<#9300 (comment)>:
> @@ -145,17 +145,15 @@ function parseJsonConfig(string, location) {
*/
function parseListConfig(string) {
const items = {};
+ const reg = /[\w-/]+/g;
no need in []
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#9300 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AARWelhJ9NuUO4l7frQ1FLncyOiUbQLnks5siWEagaJpZM4PXJh2>
.
|
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
* Revert "4.7.0" This reverts commit 439e8e6. * Revert "Build: changelog update for 4.7.0" This reverts commit 2ec62f9. * Revert "Upgrade: Espree v3.5.1 (fixes #9153) (#9314)" This reverts commit 787b78b. * Revert "Update: run rules after `node.parent` is already set (fixes #9122) (#9283)" This reverts commit 1488b51. * Revert "Docs: fix wrong config in max-len example. (#9309)" This reverts commit 4431d68. * Revert "Chore: Revert "avoid handling Rules instances in config-validator" (#9295)" This reverts commit 9d1df92. * Revert "Docs: Fix code snippet to refer to the correct option (#9313)" This reverts commit 7d24dde. * Revert "�Chore: rewrite parseListConfig for a small perf gain. (#9300)" This reverts commit 12388d4.
What is the purpose of this pull request? (put an "X" next to item)
[x] Other, please explain:
What changes did you make? (Give an overview)
Is there anything you'd like reviewers to focus on?
perf tests result (~1.7x):