FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib: refactor code with startsWith/endsWith by JacksonTian · Pull Request #5753 · nodejs/node · GitHub

/ node Public

lib: refactor code with startsWith/endsWith - #5753

Closed
JacksonTian wants to merge 1 commit into
nodejs:masterfrom
JacksonTian:starts/ends
ConversationCommits (1)ChecksFiles changed
Closed

lib: refactor code with startsWith/endsWith#5753
JacksonTian wants to merge 1 commit into
nodejs:masterfrom
JacksonTian:starts/ends

Conversation

JacksonTian commented Mar 17, 2016

Copy link
Copy Markdown
Contributor

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    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.

mscdex commented Mar 17, 2016

Copy link
Copy Markdown
Contributor

Did you run the relevant benchmarks before and after these changes?

JacksonTian commented Mar 17, 2016

Copy link
Copy Markdown
Contributor Author

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 Issues and PRs related to general changes in the lib or src directory. label Mar 17, 2016
benjamingr reviewed Mar 17, 2016
View reviewed changes
Comment thread lib/cluster.js Outdated

benjamingr Mar 17, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

These can probably be arrow functions.

JacksonTian commented Mar 21, 2016

Copy link
Copy Markdown
Contributor Author

Hi @ronkorving @benjamingr , I updated the PR with your suggestions. Thanks.

ronkorving reviewed Mar 21, 2016
View reviewed changes
Comment thread lib/tls.js
function regexpify(host, wildcards) {
// Add trailing dot (make hostnames uniform)
if (!/\.$/.test(host)) host += '.';
if (!host || !host.endsWith('.')) host += '.';

ronkorving Mar 21, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What does the !host check add?

JacksonTian Mar 22, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

the host maybe undefined.

ronkorving Mar 22, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can it really? In that case, the old code would have produced "undefined.". Does that code path really exist?

JacksonTian Mar 22, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

yes. If no !host path, make test will fails.

ronkorving Mar 22, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

JacksonTian Mar 22, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We need it here.

ronkorving Mar 22, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah, my bad. I get it now.

benjamingr commented Mar 22, 2016

Copy link
Copy Markdown
Member

CI https://ci.nodejs.org/job/node-test-pull-request/2016/console

JacksonTian commented Mar 22, 2016

Copy link
Copy Markdown
Contributor Author

The windows is unhappy, I forget to check the length of string.

Updated it. Would you please run CI again.

benjamingr commented Mar 22, 2016

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/2018/console

jasnell commented Mar 22, 2016

Copy link
Copy Markdown
Member

There were multiple CI failures that appear unrelated but should be investigated before this lands.

benjamingr commented Mar 22, 2016

Copy link
Copy Markdown
Member

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

benjamingr commented Mar 22, 2016

Copy link
Copy Markdown
Member

The CI failure seems unrelated to this. Going to land.

benjamingr commented Mar 22, 2016

Copy link
Copy Markdown
Member

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.

benjamingr commented Mar 23, 2016

Copy link
Copy Markdown
Member

Ok, reviewed now and LGTM. Ping @nodejs/collaborators I'd love this to get a second review.

mscdex reviewed Mar 23, 2016
View reviewed changes
Comment thread lib/cluster.js Outdated

mscdex Mar 23, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The indentation here is slightly off by two spaces.

lib: refactor code with startsWith/endsWith
99dd39e 
reduce using RegExp for string test.

JacksonTian commented Mar 23, 2016

Copy link
Copy Markdown
Contributor Author

Thanks @benjamingr , fixed it.

benjamingr commented Mar 23, 2016

Copy link
Copy Markdown
Member

Thanks! Landed in 91466b8

benjamingr closed this Mar 23, 2016
benjamingr pushed a commit that referenced this pull request Mar 23, 2016
lib: refactor code with startsWith/endsWith
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>
JacksonTian deleted the starts/ends branch March 23, 2016 14:38
evanlucas pushed a commit that referenced this pull request Mar 30, 2016
lib: refactor code with startsWith/endsWith
a7710b0 
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>
evanlucas mentioned this pull request Mar 31, 2016
Closed
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
lib: refactor code with startsWith/endsWith
e644eb3 
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>
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
lib: refactor code with startsWith/endsWith
16a53e3 
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>
MylesBorins pushed a commit that referenced this pull request Apr 8, 2016
lib: refactor code with startsWith/endsWith
c4103b1 
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>
MylesBorins mentioned this pull request Apr 11, 2016
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL