doc: clarify that new URL().port could be an empty string. by mcollina · Pull Request #22232 · nodejs/node

mcollina · 2018-08-10T13:56:55Z

Checklist

documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-08-10T13:56:56Z

@mcollina build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/490/pipeline

jasnell · 2018-08-10T14:08:45Z

May want to include all of the special cases here: https://url.spec.whatwg.org/#url-miscellaneous

mcollina · 2018-08-10T14:50:22Z

@jasnell PTAL.

Trott

LGTM if lint-md passes.

@nodejs/documentation

Trott · 2018-08-10T16:43:52Z

doc/api/url.md

Optional nit: and in that case -> in which case

Trott · 2018-08-10T16:44:13Z

doc/api/url.md

Optional nit: Remove the Here are the supported protocols: sentence and end the previous sentence with :

Trott · 2018-08-10T16:46:27Z

Not necessarily for this PR but might this material be better closer to this sentence:

Setting the value to the default port of the URL objects given protocol will result in the port value becoming the empty string ('').

mcollina · 2018-08-13T10:00:11Z

@Trott I've applied your nits and do a more substantial change, PTAL.

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

BridgeAR

LGTM with the comments addressed.

BridgeAR · 2018-08-13T10:14:54Z

doc/api/url.md

BridgeAR · 2018-08-13T10:17:09Z

doc/api/url.md

-Otherwise, or i. +I

The sentence seems independent even without the Otherwise.

Maybe Otherwise, or if the number -> If the resulting number?

Trott · 2018-08-13T20:58:11Z

doc/api/url.md

Micro-nit, non-blocking: may be set as either a number or as a string containing a number -> may be a number or a string containing a number

Trott · 2018-08-13T21:00:24Z

Left a few more optional nits, but the text looks good to me. Not leaving an approval because I don't know anything about this behavior, although I trust that it is being described accurately.

mcollina · 2018-08-14T09:31:23Z

PTAL.

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/555

BridgeAR

LGTM

BridgeAR · 2018-08-14T10:54:19Z

doc/api/url.md


 Gets and sets the port portion of the URL.

+The port value may be a number or a string containing a number in the range


Nit: remove containing a number

@BridgeAR I disagree, I don't think it's clear enough without that. A string cannot be in a range of numbers.

Yeah, you are probably right.

mcollina · 2018-08-17T11:01:07Z

Landed in 2ce0380

PR-URL: #22232 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: George Adams <george.adams@uk.ibm.com>

mcollina requested a review from jasnell August 10, 2018 13:56

nodejs-github-bot added doc Issues and PRs related to the documentations. url Issues and PRs related to the legacy built-in url module. labels Aug 10, 2018

BridgeAR approved these changes Aug 10, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 10, 2018

mcollina force-pushed the url-doc-change branch from 3a3e5a7 to 7fd36cc Compare August 10, 2018 14:50

jasnell approved these changes Aug 10, 2018

View reviewed changes

Trott reviewed Aug 10, 2018

View reviewed changes

doc/api/url.md Outdated

Copy link

Member

Trott Aug 10, 2018

Choose a reason for hiding this comment

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

Optional nit: and in that case -> in which case

Trott reviewed Aug 10, 2018

View reviewed changes

doc/api/url.md Outdated

Copy link

Member

Trott Aug 10, 2018

Choose a reason for hiding this comment

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

Optional nit: Remove the Here are the supported protocols: sentence and end the previous sentence with :

lpinca approved these changes Aug 10, 2018

View reviewed changes

trivikr approved these changes Aug 11, 2018

View reviewed changes

gdams approved these changes Aug 12, 2018

View reviewed changes

mcollina force-pushed the url-doc-change branch from 7fd36cc to e0a8408 Compare August 13, 2018 09:58

BridgeAR approved these changes Aug 13, 2018

View reviewed changes

Trott reviewed Aug 13, 2018

View reviewed changes

doc: clarify that new URL().port could be an empty string.

d5c6eb2

mcollina force-pushed the url-doc-change branch from e0a8408 to d5c6eb2 Compare August 14, 2018 09:30

BridgeAR approved these changes Aug 14, 2018

View reviewed changes

maclover7 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 15, 2018

mcollina closed this Aug 17, 2018

mcollina deleted the url-doc-change branch August 17, 2018 11:01

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

This was referenced Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed


		Gets and sets the port portion of the URL.

		The port value may be a number or a string containing a number in the range

Uh oh!

Conversation

mcollina commented Aug 10, 2018

Checklist

Uh oh!

nodejs-github-bot commented Aug 10, 2018

Uh oh!

jasnell commented Aug 10, 2018

Uh oh!

mcollina commented Aug 10, 2018

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Aug 10, 2018

Uh oh!

mcollina commented Aug 13, 2018

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Aug 14, 2018

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina commented Aug 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Trott commented Aug 13, 2018 •

edited

Loading