Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: add URL.format() example #18888

Closed
wants to merge 4 commits into from

Conversation

@zeke
Copy link
Member

commented Feb 20, 2018

This PR adds an example use of the URL.format() method.

Resolves #18887

Checklist
Affected core subsystem(s)
  • doc
  • url

@zeke zeke changed the title docs: add URL.format() example doc: add URL.format() example Feb 20, 2018

Example:

```js
const URL = require('url');

This comment has been minimized.

Copy link
@Trott

Trott Feb 20, 2018

Member

Although it appears in other examples in this doc, I think in general we tend to leave out the require() for the module. I'm fine either way.

This comment has been minimized.

Copy link
@Trott

Trott Feb 20, 2018

Member

Oh! But we do use url and reserve URL for the newer url.URL. So maybe remove it and use url rather than URL below.

@addaleax
Copy link
Member

left a comment

LGTM with @Trott’s suggestion, in particular because URL is now available with that name on the global object :)

@Trott

This comment has been minimized.

Copy link
Member

commented Feb 20, 2018

LGTM once we get the URL/url stuff sorted out.

@iwko

iwko approved these changes Feb 20, 2018

@zeke

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2018

Took out the require and changed URL to url to avoid new API / old API confusion. Thanks for the quick feedback!

@lpinca

lpinca approved these changes Feb 20, 2018

@@ -948,6 +948,22 @@ changes:
The `url.format()` method returns a formatted URL string derived from
`urlObject`.

Example:

This comment has been minimized.

Copy link
@Trott

Trott Feb 20, 2018

Member

Nit: You can remove Example: too. It's obvious that it is an example.

@Trott

Trott approved these changes Feb 20, 2018

@BridgeAR
Copy link
Member

left a comment

LGTM but I think it would be good to switch to "Prints".

}
});
// => 'https://example.com/some/path?page=1&format=json'

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Feb 21, 2018

Member

Other places in the file use Prints or just show the output below. I personally think a "Prints" would be better than =>.

This comment has been minimized.

Copy link
@zeke

zeke Feb 21, 2018

Author Member

I did a search in the docs and found several instances of // => symbolizing a returned value. Also, this method doesn't actually print a value, it just returns it.

This comment has been minimized.

Copy link
@zeke

zeke Feb 21, 2018

Author Member

I would be open to just doing // https://.... That was another "convention" I found when looking at existing code examples in Node's docs.

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Feb 21, 2018

Member

In this case I only spoke about this specific document. And yes, // => is still used some times but it is not really common as far as I know.

So I personally would say using // 'https:... would be good.

```js
url.format({
protocol: 'https',
hostname: `example.com`,

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Feb 21, 2018

Contributor

Are the backticks intended? If so, are not they a bit confusing here? With them, a template literal/a tag function/other quotes inside are usually expected.

This comment has been minimized.

Copy link
@zeke

zeke Feb 21, 2018

Author Member

Good catch, @vsemozhetbyt. Backticks were not intentional there. Fixed.

@tniessen

This comment has been minimized.

Copy link
Member

commented Mar 4, 2018

@apapirovski

This comment has been minimized.

Copy link
Member

commented Mar 4, 2018

Landed in 1572a5b

@apapirovski apapirovski closed this Mar 4, 2018

apapirovski added a commit that referenced this pull request Mar 4, 2018

doc: add URL.format() example
PR-URL: #18888
Fixes: #18887
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

@zeke zeke deleted the zeke:add-url-format-example-to-docs branch Mar 4, 2018

addaleax added a commit to addaleax/node that referenced this pull request Mar 5, 2018

doc: add URL.format() example
PR-URL: nodejs#18888
Fixes: nodejs#18887
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

addaleax added a commit to addaleax/node that referenced this pull request Mar 5, 2018

doc: add URL.format() example
PR-URL: nodejs#18888
Fixes: nodejs#18887
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

@MylesBorins MylesBorins referenced this pull request Mar 6, 2018

Merged

v9.8.0 proposal #19181

MayaLekova added a commit to MayaLekova/node that referenced this pull request May 8, 2018

doc: add URL.format() example
PR-URL: nodejs#18888
Fixes: nodejs#18887
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

jasnell added a commit to jasnell/node that referenced this pull request Aug 17, 2018

doc: add URL.format() example
PR-URL: nodejs#18888
Fixes: nodejs#18887
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

MylesBorins added a commit that referenced this pull request Sep 6, 2018

doc: add URL.format() example
Backport-PR-URL: #22380
PR-URL: #18888
Fixes: #18887
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

@MylesBorins MylesBorins referenced this pull request Sep 6, 2018

Merged

v8.12.0 proposal #21593

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.