doc: general improvements to url.md copy #6904

jasnell · 2016-05-20T20:12:11Z

Checklist

documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

doc (url)

Description of change

General improvements to url.md copy

@nodejs/documentation @mscdex

eljefedelrodeodeljefe · 2016-05-20T20:22:13Z

doc/api/url.md

+`'http://user:pass@host.com:8080/p/a/t/h?query=string#hash'` is used to
+illustrate each.
+
+### `href`


Curious how those render in HTML. The GH markdown doesn't look pretty here, but I think there is no alternative. ff

eljefedelrodeodeljefe · 2016-05-20T20:26:32Z

Semantically very good, with one big concern inline. Rubber stamp LGTM if there we don't have better ideas, since it's anyhow an improvement.

addaleax · 2016-05-20T21:24:34Z

doc/api/url.md

+* A new empty string `result` is created.
+* If `urlObj.protocol` is a string, it is appended as-is to `result`.
+* Otherwise, if `urlObj.protocol` is not `undefined` and is not a string, an
+  [`Error`][] is thrown.


I don’t think the reference for [Error][] is defined yet

addaleax · 2016-05-20T21:53:03Z

Bonus points if you or someone else manages to get the diagram from nodejs/node-v0.x-archive#8755 (comment) in here… I think someone said something about wanting to make an SVG out of it and including it here a few weeks ago. 😄

jasnell · 2016-05-20T23:41:43Z

@addaleax ... who needs SVG!!
@eljefedelrodeodeljefe ... I'll see if I can work on it a bit more to make it easier to read.

addaleax · 2016-05-21T22:44:25Z

LGTM

jasnell · 2016-05-23T17:02:11Z

@eljefedelrodeodeljefe ... I'm thinking of going ahead with landing this and working on making additional improvements later on in another round. Would that work for you?

eljefedelrodeodeljefe · 2016-05-23T17:48:27Z

Sure LGTM then

thefourtheye · 2016-05-23T18:42:11Z

doc/api/url.md

+  begins with one of `http`, `https`, `ftp`, `gopher`, or `file`, or
+  `urlObject.protocol` is `undefined`, the literal string `//` will be appended
+  to `result`.
+* If the value of the `urlObject.auth` property is `truthy`, and either


truthy is not code component.

thefourtheye · 2016-05-23T19:24:11Z

LGTM

jasnell · 2016-05-23T19:52:19Z

@thefourtheye ... updated to address the truthy nit

jasnell · 2016-05-23T20:27:19Z

@mscdex ... can I ask you to please double check this before I land?

mscdex · 2016-05-24T06:12:34Z

doc/api/url.md


-`'http://user:pass@host.com:8080/p/a/t/h?query=string#hash'`
+A URL String is a structured string containing multiple meaningful components. 
+When parsed, a URL Object is returned containing properties for each of these


lowercase object here?

jasnell · 2016-05-24T15:01:18Z

@mscdex ... updated! thank you for the review!

mscdex · 2016-05-24T16:29:24Z

doc/api/url.md

+|          ||           | hostname | port | pathname | search       |       |
+|          ||           |          |      |          +-+------------+       |
+|          ||           |          |      |          | | query      |       |
+" http:    // user:pass @ host.com : 8080 | /p/a/t/h |?query=string | #hash "


I think the | between the port and pathname should just be removed, same with the | between pathname and search, and with the | between search and hash.

Also placing query=string completely inside the query section is clearer.

Like this:

| || | | | | | query | | " http: // user:pass @ host.com : 8080 /p/a/t/h ? query=string #hash "

benjamingr · 2016-05-24T16:37:52Z

LGTM although I agree with @mscdex 's suggestions

mscdex · 2016-05-24T16:42:51Z

doc/api/url.md


-The following methods are provided by the URL module:
+No decoding of the query string is performed.


This needs to clarified to say that this is only the case when the query string isn't parsed. When it is parsed, keys and values are decoded.

jasnell · 2016-05-24T18:01:17Z

@mscdex ... updated!

mscdex · 2016-05-24T20:03:57Z

doc/api/url.md

-  be set to an object returned by the `querystring` module's `parse()` method.
-  If `false`, the `query` property on the returned URL object will be an
+  be set to an object returned by the [`querystring`][] module's `parse()`
+  method. If `false`, the `query` property on the returned URL object will be an


minor nit: make "function"/"method" usage consistent

General cleanup and restructuring of the doc. Added additional detail to how URLs are serialized.

jasnell · 2016-05-25T15:46:40Z

@mscdex ... updated, simply did s/function/method.
Squashed the commits. Should be good to go.

jasnell · 2016-05-26T19:21:06Z

@mscdex... LGTY?

mscdex · 2016-05-26T22:13:02Z

LGTM

General cleanup and restructuring of the doc. Added additional detail to how URLs are serialized. PR-URL: #6904 Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

jasnell · 2016-05-27T01:10:17Z

Landed in dbdea02. Thanks all!

General cleanup and restructuring of the doc. Added additional detail to how URLs are serialized. PR-URL: nodejs#6904 Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

General cleanup and restructuring of the doc. Added additional detail to how URLs are serialized. PR-URL: #6904 Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

MylesBorins · 2016-07-11T21:44:37Z

added don't land label for v4.x

Please feel free to backport

jasnell added doc Issues and PRs related to the documentations. url Issues and PRs related to the legacy built-in url module. labels May 20, 2016

eljefedelrodeodeljefe reviewed May 20, 2016
View reviewed changes

addaleax reviewed May 20, 2016
View reviewed changes

jasnell force-pushed the doc-url-copy branch from 9a2c71d to 4535d3e Compare May 23, 2016 17:01

thefourtheye reviewed May 23, 2016
View reviewed changes

mscdex reviewed May 24, 2016
View reviewed changes

doc: general improvements to url.md copy

1361514

General cleanup and restructuring of the doc. Added additional detail to how URLs are serialized.

jasnell force-pushed the doc-url-copy branch from d95fd2a to 1361514 Compare May 25, 2016 15:46

jasnell closed this May 27, 2016

MylesBorins mentioned this pull request Jul 11, 2016

Audit commits not found on v4.x-staging #7661

Closed

MylesBorins added the dont-land-on-v4.x label Jul 11, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: general improvements to url.md copy #6904

doc: general improvements to url.md copy #6904

jasnell commented May 20, 2016 •

edited

Loading

eljefedelrodeodeljefe May 20, 2016

eljefedelrodeodeljefe commented May 20, 2016

addaleax May 20, 2016

addaleax commented May 20, 2016

jasnell commented May 20, 2016

addaleax commented May 21, 2016

jasnell commented May 23, 2016

eljefedelrodeodeljefe commented May 23, 2016

thefourtheye May 23, 2016

thefourtheye commented May 23, 2016

jasnell commented May 23, 2016

jasnell commented May 23, 2016

mscdex May 24, 2016

jasnell commented May 24, 2016

mscdex May 24, 2016 •

edited

Loading

benjamingr commented May 24, 2016

mscdex May 24, 2016 •

edited

Loading

jasnell commented May 24, 2016

mscdex May 24, 2016

jasnell commented May 25, 2016

jasnell commented May 26, 2016

mscdex commented May 26, 2016

jasnell commented May 27, 2016

MylesBorins commented Jul 11, 2016


		The following methods are provided by the URL module:
		No decoding of the query string is performed.

doc: general improvements to url.md copy #6904

doc: general improvements to url.md copy #6904

Conversation

jasnell commented May 20, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

eljefedelrodeodeljefe May 20, 2016

Choose a reason for hiding this comment

eljefedelrodeodeljefe commented May 20, 2016

addaleax May 20, 2016

Choose a reason for hiding this comment

addaleax commented May 20, 2016

jasnell commented May 20, 2016

addaleax commented May 21, 2016

jasnell commented May 23, 2016

eljefedelrodeodeljefe commented May 23, 2016

thefourtheye May 23, 2016

Choose a reason for hiding this comment

thefourtheye commented May 23, 2016

jasnell commented May 23, 2016

jasnell commented May 23, 2016

mscdex May 24, 2016

Choose a reason for hiding this comment

jasnell commented May 24, 2016

mscdex May 24, 2016 • edited Loading

Choose a reason for hiding this comment

benjamingr commented May 24, 2016

mscdex May 24, 2016 • edited Loading

Choose a reason for hiding this comment

jasnell commented May 24, 2016

mscdex May 24, 2016

Choose a reason for hiding this comment

jasnell commented May 25, 2016

jasnell commented May 26, 2016

mscdex commented May 26, 2016

jasnell commented May 27, 2016

MylesBorins commented Jul 11, 2016

jasnell commented May 20, 2016 •

edited

Loading

mscdex May 24, 2016 •

edited

Loading

mscdex May 24, 2016 •

edited

Loading