lib: refactor code with startsWith/endsWith#5753
lib: refactor code with startsWith/endsWith#5753JacksonTian wants to merge 1 commit intonodejs:masterfrom
Conversation
|
Did you run the relevant benchmarks before and after these changes? |
|
I run the os.tmpdir after change, the benchmarks have no effect. Should I do a benchmark for RegExp vs startsWith/endsWith? |
mscdex
added
the
lib / src
lib/cluster.js
Outdated
There was a problem hiding this comment.
These can probably be arrow functions.
JacksonTian
force-pushed
the
starts/ends
branch
from
b5edadc to
28da97f
Compare
|
Hi @ronkorving @benjamingr , I updated the PR with your suggestions. Thanks. |
| function regexpify(host, wildcards) { | ||
| // Add trailing dot (make hostnames uniform) | ||
| if (!/\.$/.test(host)) host += '.'; | ||
| if (!host || !host.endsWith('.')) host += '.'; |
There was a problem hiding this comment.
What does the !host check add?
There was a problem hiding this comment.
the host maybe undefined.
There was a problem hiding this comment.
Can it really? In that case, the old code would have produced "undefined.". Does that code path really exist?
There was a problem hiding this comment.
yes. If no !host path, make test will fails.
There was a problem hiding this comment.
Well, it wouldn't have blown up, but would've turned undefined into "undefined", before appending a period to it. My point is, this was never a bug, so do we need that !host check?
There was a problem hiding this comment.
We need it here.
There was a problem hiding this comment.
Ah, my bad. I get it now.
JacksonTian
force-pushed
the
starts/ends
branch
from
28da97f to
291bc00
Compare
|
The windows is unhappy, I forget to check the length of string. Updated it. Would you please run CI again. |
|
There were multiple CI failures that appear unrelated but should be investigated before this lands. |
|
They say insanity is doing the same thing twice and expecting different results but I've had a long day so I'll give it another shot before investigating the CI results further: https://ci.nodejs.org/job/node-test-pull-request/2029/console |
|
The CI failure seems unrelated to this. Going to land. |
|
Wait, I just realized no one has given this a LGTM yet and I'm not completely sure about all the code involved myself - so I'd rather wait for another collaborator to LGTM it before landing. |
|
Ok, reviewed now and LGTM. Ping @nodejs/collaborators I'd love this to get a second review. |
lib/cluster.js
Outdated
There was a problem hiding this comment.
The indentation here is slightly off by two spaces.
reduce using RegExp for string test.
JacksonTian
force-pushed
the
starts/ends
branch
from
291bc00 to
99dd39e
Compare
|
Thanks @benjamingr , fixed it. |
|
Thanks! Landed in 91466b8 |
reduce using RegExp for string test. This pull reuqest replaces various usages of regular expressions in favor of the ES2015 startsWith and endsWith methods. PR-URL: #5753 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
reduce using RegExp for string test. This pull reuqest replaces various usages of regular expressions in favor of the ES2015 startsWith and endsWith methods. PR-URL: #5753 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
reduce using RegExp for string test. This pull reuqest replaces various usages of regular expressions in favor of the ES2015 startsWith and endsWith methods. PR-URL: #5753 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
reduce using RegExp for string test. This pull reuqest replaces various usages of regular expressions in favor of the ES2015 startsWith and endsWith methods. PR-URL: #5753 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
reduce using RegExp for string test. This pull reuqest replaces various usages of regular expressions in favor of the ES2015 startsWith and endsWith methods. PR-URL: #5753 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
6 participants
Pull Request check-list
Please make sure to review and check all of these items:
make -j8 test(UNIX) orvcbuild test nosign(Windows) pass withthis change (including linting)?
test (or a benchmark) included?
existing APIs, or introduces new ones)?
NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.
Affected core subsystem(s)
lib
Description of change
reduce using REGExp for string test.