doc: Add URL instance as options argument with http.request and https… #13405

vladimir-trifonov · 2017-06-02T15:26:38Z

….request

Fixes: #13383

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

mscdex · 2017-06-02T15:28:02Z

Nit: First line of the commit message is too long

vsemozhetbyt · 2017-06-02T15:32:41Z

Thank you for the contribution!

A nit: we usually wrap lines in docs at 80 characters according to the style guide.

vsemozhetbyt · 2017-06-02T15:42:15Z

doc/api/http.md

@@ -1677,8 +1685,9 @@ added: v0.3.6
 Node.js maintains several connections per server to make HTTP requests.
 This function allows one to transparently issue requests.

-`options` can be an object or a string. If `options` is a string, it is
-automatically parsed with [`url.parse()`][].
+`options` can be an object, a string, or a [`URL`][] object. If `options` is a string, it is


If I get this right, [`URL`] should be added to the link references at the end of the file.

vsemozhetbyt · 2017-06-02T15:47:08Z

doc/api/http.md

+Example using options from [`URL`][]:
+
+```js
+const options = new URL('https://abc:xyz@example.com');


It seems this should be http:

tniessen · 2017-06-02T15:45:39Z

doc/api/http.md

-`options` can be an object or a string. If `options` is a string, it is
-automatically parsed with [`url.parse()`][].
+`options` can be an object, a string, or a [`URL`][] object. If `options` is a string, it is
+automatically parsed with [`url.parse()`][]. If it's a [`URL`][] object, it will be automatically


I don't know how consistent we are about this in the docs, but I would prefer "it is" instead of "it's".

tniessen · 2017-06-02T15:47:06Z

doc/api/https.md

-`options` can be an object or a string. If `options` is a string, it is
-automatically parsed with [`url.parse()`][].
+`options` can be an object, a string, or a [`URL`][] object. If `options` is a string, it is
+automatically parsed with [`url.parse()`][]. If it's a [`URL`][] object, it will be automatically


tniessen · 2017-06-02T15:47:09Z

doc/api/https.md

-`options` can be an object or a string. If `options` is a string, it is
-automatically parsed with [`url.parse()`][].
+`options` can be an object, a string, or a [`URL`][] object. If `options` is a string, it is
+automatically parsed with [`url.parse()`][]. If it's a [`URL`][] object,


vsemozhetbyt · 2017-06-02T15:49:40Z

doc/api/https.md

  [`https.request()`][], with the `method` always set to `GET`.
 - `callback` {Function}

 Like [`http.get()`][] but for HTTPS.

-`options` can be an object or a string. If `options` is a string, it is
-automatically parsed with [`url.parse()`][].
+`options` can be an object, a string, or a [`URL`][] object. If `options` is a string, it is


The same: [`URL`] should be added to the link references at the end of the file

vsemozhetbyt · 2017-06-02T15:56:43Z

Preliminary linter CI: https://ci.nodejs.org/job/node-test-linter/9557/

vladimir-trifonov · 2017-06-02T17:47:59Z

Thanks for the comments. I fixed all the requested changes.

jasnell

LGTM with the nits already reported fixed.

vsemozhetbyt · 2017-06-02T18:01:36Z

@vladimir-trifonov Thank you. There are some more nits:

Some lines still exceed 80 characters.
As you can see, link references are usually sorted alphabetically and case-sensitively.

vladimir-trifonov · 2017-06-02T18:28:50Z

@vsemozhetbyt Sorry about that. You are right. I fixed them.

vsemozhetbyt · 2017-06-02T18:35:50Z

Linter CI: https://ci.nodejs.org/job/node-test-linter/9563/

cjihrig

LGTM, but the code samples may want to show the necessary require() code to get the URL constructor.

Fixes: nodejs#13383

vladimir-trifonov · 2017-06-04T06:31:11Z

Added the require() code :)

PR-URL: #13405 Fixes: #13383 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

TimothyGu · 2017-06-07T05:46:31Z

Landed in 60d14c8 with some minor wording tweaks. @vladimir-trifonov Thanks, and welcome to Node.js!

PR-URL: #13405 Fixes: #13383 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

MylesBorins · 2017-07-17T17:50:59Z

I don't believe this should land on v6.x

Please correct me if wrong

vladimir-trifonov · 2017-07-18T06:45:20Z

@MylesBorins It's for v7.5.0 if I'm not wrong.

jasnell · 2017-07-18T06:46:14Z

Correct, it should not go to 6.x

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jun 2, 2017

mscdex added http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. labels Jun 2, 2017

vsemozhetbyt reviewed Jun 2, 2017

View reviewed changes

tniessen reviewed Jun 2, 2017

View reviewed changes

vsemozhetbyt reviewed Jun 2, 2017

View reviewed changes

vladimir-trifonov force-pushed the issue13383 branch 3 times, most recently from 600d1b3 to ce1e1a9 Compare June 2, 2017 17:43

jasnell approved these changes Jun 2, 2017

View reviewed changes

vladimir-trifonov force-pushed the issue13383 branch from ce1e1a9 to 465261e Compare June 2, 2017 18:25

vsemozhetbyt approved these changes Jun 2, 2017

View reviewed changes

tniessen approved these changes Jun 2, 2017

View reviewed changes

cjihrig approved these changes Jun 3, 2017

View reviewed changes

lpinca approved these changes Jun 3, 2017

View reviewed changes

doc: Add URL argument with http/https request

b3e3ff0

Fixes: nodejs#13383

vladimir-trifonov force-pushed the issue13383 branch from 465261e to b3e3ff0 Compare June 4, 2017 06:29

hiroppy approved these changes Jun 4, 2017

View reviewed changes

TimothyGu approved these changes Jun 7, 2017

View reviewed changes

TimothyGu closed this Jun 7, 2017

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the dont-land-on-v6.x label Jul 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: Add URL instance as options argument with http.request and https… #13405

doc: Add URL instance as options argument with http.request and https… #13405

vladimir-trifonov commented Jun 2, 2017

mscdex commented Jun 2, 2017

vsemozhetbyt commented Jun 2, 2017

vsemozhetbyt Jun 2, 2017

vsemozhetbyt Jun 2, 2017

tniessen Jun 2, 2017

tniessen Jun 2, 2017

tniessen Jun 2, 2017

vsemozhetbyt Jun 2, 2017

vsemozhetbyt commented Jun 2, 2017

vladimir-trifonov commented Jun 2, 2017

jasnell left a comment

vsemozhetbyt commented Jun 2, 2017

vladimir-trifonov commented Jun 2, 2017 •

edited

Loading

vsemozhetbyt commented Jun 2, 2017

cjihrig left a comment

vladimir-trifonov commented Jun 4, 2017

TimothyGu commented Jun 7, 2017

MylesBorins commented Jul 17, 2017

vladimir-trifonov commented Jul 18, 2017

jasnell commented Jul 18, 2017

doc: Add URL instance as options argument with http.request and https… #13405

doc: Add URL instance as options argument with http.request and https… #13405

Conversation

vladimir-trifonov commented Jun 2, 2017

Checklist

Affected core subsystem(s)

mscdex commented Jun 2, 2017

vsemozhetbyt commented Jun 2, 2017

vsemozhetbyt Jun 2, 2017

Choose a reason for hiding this comment

vsemozhetbyt Jun 2, 2017

Choose a reason for hiding this comment

tniessen Jun 2, 2017

Choose a reason for hiding this comment

tniessen Jun 2, 2017

Choose a reason for hiding this comment

tniessen Jun 2, 2017

Choose a reason for hiding this comment

vsemozhetbyt Jun 2, 2017

Choose a reason for hiding this comment

vsemozhetbyt commented Jun 2, 2017

vladimir-trifonov commented Jun 2, 2017

jasnell left a comment

Choose a reason for hiding this comment

vsemozhetbyt commented Jun 2, 2017

vladimir-trifonov commented Jun 2, 2017 • edited Loading

vsemozhetbyt commented Jun 2, 2017

cjihrig left a comment

Choose a reason for hiding this comment

vladimir-trifonov commented Jun 4, 2017

TimothyGu commented Jun 7, 2017

MylesBorins commented Jul 17, 2017

vladimir-trifonov commented Jul 18, 2017

jasnell commented Jul 18, 2017

vladimir-trifonov commented Jun 2, 2017 •

edited

Loading