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

lib: fix validateport error message when allowZero is false by rickyes · Pull Request #32861 · nodejs/node · GitHub

/ node Public

lib: fix validateport error message when allowZero is false - #32861

Closed
rickyes wants to merge 1 commit into
nodejs:masterfrom
rickyes:fix-validateport-errmsg
ConversationCommits (1)ChecksFiles changed
Closed

lib: fix validateport error message when allowZero is false#32861
rickyes wants to merge 1 commit into
nodejs:masterfrom
rickyes:fix-validateport-errmsg

Conversation

rickyes commented Apr 15, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

fixes: #32857

/cc @jasnell

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Apr 15, 2020
rickyes force-pushed the fix-validateport-errmsg branch from 8fc220c to 7cb9849 Compare April 15, 2020 06:08
rickyes changed the title lib: fix validateport error message when allowZero = false lib: fix validateport error message when allowZero is false Apr 15, 2020

rickyes commented Apr 15, 2020

Copy link
Copy Markdown
Contributor Author

It seems that there is a problem with install deps when building on windows.

Failures
 - nasm (exited 1) - nasm not installed. An error occurred during installation:
 The remote server returned an error: (503) Server Unavailable. Service Unavailable

himself65 requested changes Apr 15, 2020
View reviewed changes
Comment thread lib/internal/errors.js Outdated
rickyes force-pushed the fix-validateport-errmsg branch from 7cb9849 to b5ac0f0 Compare April 15, 2020 07:24
himself65 approved these changes Apr 15, 2020
View reviewed changes

himself65 left a comment

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

LGTM if CI passes

cjihrig approved these changes Apr 15, 2020
View reviewed changes

cjihrig left a comment

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

LGTM.

No need to change this PR, but It's kind of funny that there was discussion about making the third argument to validatePort() an object vs. a boolean for readability purposes, just to end up using a boolean here anyway. It seems like we should migrate the validatePort() arg to a boolean - it's an internal API anyway.

rickyes commented Apr 15, 2020

Copy link
Copy Markdown
Contributor Author

LGTM.

No need to change this PR, but It's kind of funny that there was discussion about making the third argument to validatePort() an object vs. a boolean for readability purposes, just to end up using a boolean here anyway. It seems like we should migrate the validatePort() arg to a boolean - it's an internal API anyway.

I think making the third parameter of validatePort() an object is actually to consider compatibility with different ranges of ports in the future.

cjihrig commented Apr 15, 2020

Copy link
Copy Markdown
Contributor

I think making the third parameter of validatePort() an object is actually to consider compatibility with different ranges of ports in the future.

It was to avoid "magical boolean values"

jasnell commented Apr 15, 2020

Copy link
Copy Markdown
Member
  • Avoiding magical booleans
  • API consistency (some of the validation function accepted options objects others did not)
  • Support for additional options in the future without changing API signature

cjihrig commented Apr 15, 2020

Copy link
Copy Markdown
Contributor

API consistency (some of the validation function accepted options objects others did not)

To be fair, more of them do not take an options object, so it introduced more inconsistency.

nodejs-github-bot commented Apr 17, 2020

Copy link
Copy Markdown
Collaborator

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

rickyes commented Apr 25, 2020

Copy link
Copy Markdown
Contributor Author

Should be ready, Can you help restart a CI run ? @jasnell

nodejs-github-bot commented Apr 25, 2020

Copy link
Copy Markdown
Collaborator

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

rickyes commented Apr 25, 2020

Copy link
Copy Markdown
Contributor Author

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

It seems that the CI run failure has nothing to do with this PR.

11:56:22 not ok 2559 parallel/test-worker-message-port-message-before-close
11:56:22   ---
11:56:22   duration_ms: 120.104
11:56:22   severity: fail
11:56:22   exitcode: -15
11:56:22   stack: |-
11:56:22     timeout

nodejs-github-bot commented Apr 25, 2020

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 28, 2020

Copy link
Copy Markdown
Collaborator

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

addaleax pushed a commit that referenced this pull request Apr 28, 2020
lib: fix validateport error message when allowZero is false
1d0b249 
PR-URL: #32861
Fixes: #32857
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

addaleax commented Apr 28, 2020

Copy link
Copy Markdown
Member

Landed in 1d0b249

addaleax closed this Apr 28, 2020
rickyes deleted the fix-validateport-errmsg branch April 28, 2020 17:24
targos pushed a commit that referenced this pull request May 4, 2020
lib: fix validateport error message when allowZero is false
542da43 
PR-URL: #32861
Fixes: #32857
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos mentioned this pull request May 4, 2020
Merged
targos pushed a commit that referenced this pull request May 7, 2020
lib: fix validateport error message when allowZero is false
c822f12 
PR-URL: #32861
Fixes: #32857
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos mentioned this pull request May 7, 2020
Merged
targos pushed a commit that referenced this pull request May 13, 2020
lib: fix validateport error message when allowZero is false
6356ad4 
PR-URL: #32861
Fixes: #32857
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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

bnoordhuis bnoordhuis approved these changes
jasnell jasnell approved these changes
cjihrig cjihrig approved these changes
BridgeAR BridgeAR approved these changes
himself65 himself65 approved these changes

Assignees

No one assigned

Labels

errors Issues and PRs related to JavaScript errors originated in Node.js core.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ERR_SOCKET_BAD_PORT's static-typed error is not affected by validatePort's allowZero value

8 participants

Back | FazBrowse Home | New Git URL