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

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

@@ -303,7 +303,19 @@ to percent-encode may vary somewhat from what the [`url.parse()`][] and

 * {string}

-Gets and sets the port portion of the URL.
+Gets and sets the port portion of the URL. It can be an empty string and in
+that case the port depends on the protocol/scheme. Here are the


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

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

doc/api/url.md

-Gets and sets the port portion of the URL.
+Gets and sets the port portion of the URL. It can be an empty string and in
+that case the port depends on the protocol/scheme. Here are the
+supported protocols:


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

+of the `URL` objects given `protocol` will result in the `port` value becoming
+the empty string (`''`).
+
+The port value can be an empty string and in which case the port depends on


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

doc/api/url.md

+
+If that string is invalid but it begins with a number, the leading number is
+assigned to `port`.
+Otherwise, or if the number lies outside the range denoted above,


-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

@@ -305,6 +305,32 @@ to percent-encode may vary somewhat from what the [`url.parse()`][] and

 Gets and sets the port portion of the URL.

+The port value may be set as either a number or as a string containing a number


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

@@ -305,6 +305,31 @@ to percent-encode may vary somewhat from what the [`url.parse()`][] and

 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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mcollina commented Aug 10, 2018

nodejs-github-bot commented Aug 10, 2018

jasnell commented Aug 10, 2018

mcollina commented Aug 10, 2018

Trott left a comment

Trott Aug 10, 2018

Trott Aug 10, 2018

Trott commented Aug 10, 2018

mcollina commented Aug 13, 2018

BridgeAR left a comment

BridgeAR Aug 13, 2018

BridgeAR Aug 13, 2018

Trott Aug 13, 2018

Trott Aug 13, 2018

Trott commented Aug 13, 2018 •

edited

Loading

mcollina commented Aug 14, 2018

BridgeAR left a comment

BridgeAR Aug 14, 2018

mcollina Aug 17, 2018

BridgeAR Aug 17, 2018

mcollina commented Aug 17, 2018

		@@ -305,6 +305,32 @@ to percent-encode may vary somewhat from what the [`url.parse()`][] and

		Gets and sets the port portion of the URL.

		The port value may be set as either a number or as a string containing a number

		@@ -305,6 +305,31 @@ to percent-encode may vary somewhat from what the [`url.parse()`][] and

		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

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

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

Conversation

mcollina commented Aug 10, 2018

Checklist

nodejs-github-bot commented Aug 10, 2018

jasnell commented Aug 10, 2018

mcollina commented Aug 10, 2018

Trott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Aug 10, 2018

mcollina commented Aug 13, 2018

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Aug 13, 2018 • edited Loading

mcollina commented Aug 14, 2018

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Aug 17, 2018

Trott commented Aug 13, 2018 •

edited

Loading