GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — lib: refactor code with startsWith/endsWith by JacksonTian · Pull Request #5753 · nodejs/node

Conversation

@JacksonTian

@JacksonTian 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@mscdex

mscdex commented Mar 17, 2016

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JacksonTian

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mscdex mscdex added the lib / src Issues and PRs involving general changes in the lib/ or src/ directories. label Mar 17, 2016
@jasnell jasnell added the lts-watch-v4.x label Mar 17, 2016
benjamingr
benjamingr reviewed Mar 17, 2016
View reviewed changes
Comment thread lib/cluster.js Outdated

@benjamingr benjamingr Mar 17, 2016

Copy link
Copy Markdown
Member

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.

These can probably be arrow functions.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@JacksonTian
JacksonTian force-pushed the starts/ends branch from b5edadc to 28da97f Compare March 21, 2016 14:59
@JacksonTian

JacksonTian commented Mar 21, 2016

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ronkorving
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 ronkorving Mar 21, 2016

Copy link
Copy Markdown
Contributor

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.

What does the !host check add?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@JacksonTian JacksonTian Mar 22, 2016

Copy link
Copy Markdown
Contributor Author

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.

the host maybe undefined.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ronkorving ronkorving Mar 22, 2016

Copy link
Copy Markdown
Contributor

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 it really? In that case, the old code would have produced "undefined.". Does that code path really exist?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@JacksonTian JacksonTian Mar 22, 2016

Copy link
Copy Markdown
Contributor Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ronkorving ronkorving Mar 22, 2016

Copy link
Copy Markdown
Contributor

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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@JacksonTian JacksonTian Mar 22, 2016

Copy link
Copy Markdown
Contributor Author

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.

We need it here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ronkorving ronkorving Mar 22, 2016

Copy link
Copy Markdown
Contributor

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.

Ah, my bad. I get it now.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@benjamingr

benjamingr commented Mar 22, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JacksonTian
JacksonTian force-pushed the starts/ends branch from 28da97f to 291bc00 Compare March 22, 2016 11:50
@JacksonTian

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr

benjamingr commented Mar 22, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr

benjamingr commented Mar 22, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mscdex
mscdex reviewed Mar 23, 2016
View reviewed changes
Comment thread lib/cluster.js Outdated

@mscdex mscdex Mar 23, 2016

Copy link
Copy Markdown
Contributor

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.

The indentation here is slightly off by two spaces.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@JacksonTian
lib: refactor code with startsWith/endsWith
99dd39e 
reduce using RegExp for string test.
@JacksonTian
JacksonTian force-pushed the starts/ends branch from 291bc00 to 99dd39e Compare March 23, 2016 13:43
@JacksonTian

JacksonTian commented Mar 23, 2016

Copy link
Copy Markdown
Contributor Author

Thanks @benjamingr , fixed it.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr

benjamingr commented Mar 23, 2016

Copy link
Copy Markdown
Member

Thanks! Landed in 91466b8
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr benjamingr closed this Mar 23, 2016
benjamingr pushed a commit that referenced this pull request Mar 23, 2016
@JacksonTian @benjamingr
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 <[email protected]>
Reviewed-By: Brian White <[email protected]>
@JacksonTian
JacksonTian deleted the starts/ends branch March 23, 2016 14:38
evanlucas pushed a commit that referenced this pull request Mar 30, 2016
@JacksonTian @evanlucas
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 <[email protected]>
Reviewed-By: Brian White <[email protected]>
@evanlucas evanlucas mentioned this pull request Mar 31, 2016
v5.10.0 Release Proposal #5970
Closed
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
@JacksonTian @evanlucas
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 <[email protected]>
Reviewed-By: Brian White <[email protected]>
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
@JacksonTian @evanlucas
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 <[email protected]>
Reviewed-By: Brian White <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 8, 2016
@JacksonTian Myles Borins
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 <[email protected]>
Reviewed-By: Brian White <[email protected]>
@MylesBorins MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Apr 8, 2016
@MylesBorins MylesBorins mentioned this pull request Apr 11, 2016
V4.4.3 proposal #6149
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

lib / src Issues and PRs involving general changes in the lib/ or src/ directories.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@JacksonTian @mscdex @benjamingr @jasnell @ronkorving @MylesBorins
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.