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.

Sign up for GitHub

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

Jump to bottom

doc: remove port from example in url.hostname #45927

Merged
merged 1 commit into from
Dec 24, 2022
Merged

doc: remove port from example in url.hostname #45927

merged 1 commit into from
Dec 24, 2022

Conversation

deokjinkim
Copy link
Contributor

@deokjinkim deokjinkim commented Dec 21, 2022 •
edited
Loading

If port is used for url.hostname, url.hostname is not working. So remove port from example in url.hostname.

@nodejs-github-bot 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 Dec 21, 2022
aduh95
aduh95 reviewed Dec 21, 2022
View reviewed changes
doc/api/url.md Show resolved Hide resolved
aduh95
aduh95 reviewed Dec 21, 2022
View reviewed changes
doc/api/url.md Outdated Show resolved Hide resolved
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 21, 2022
@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Dec 21, 2022
lpinca
lpinca reviewed Dec 21, 2022
View reviewed changes
doc/api/url.md Outdated
@@ -251,15 +251,15 @@ const myURL = new URL('https://example.org:81/foo');
console.log(myURL.hostname);
// Prints example.org

// Setting the hostname does not change the port
// Setting an invalid hostname is ignored:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Setting an invalid hostname is ignored:
// Setting the hostname does not change the port

The sentence on line 265 already clarifies that invalid host name values are ignored.

doc/api/url.md Outdated
@@ -251,15 +251,15 @@ const myURL = new URL('https://example.org:81/foo');
console.log(myURL.hostname);
// Prints example.org

// Setting the hostname does not change the port
// Setting an invalid hostname is ignored:
myURL.hostname = 'example.com:82';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
myURL.hostname = 'example.com:82';
myURL.hostname = 'example.com';

doc/api/url.md Outdated
myURL.hostname = 'example.com:82';
console.log(myURL.href);
// Prints https://example.com:81/foo
// Prints https://example.org:81/foo
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Prints https://example.org:81/foo
// Prints https://example.com:81/foo

lpinca
lpinca reviewed Dec 21, 2022
View reviewed changes
doc/api/url.md Outdated

// Use myURL.host to change the hostname and port
myURL.host = 'example.org:82';
myURL.host = 'example.com:82';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
myURL.host = 'example.com:82';
myURL.host = 'example.org:82';

doc/api/url.md Outdated
console.log(myURL.href);
// Prints https://example.org:82/foo
// Prints https://example.com:82/foo
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Prints https://example.com:82/foo
// Prints https://example.org:82/foo

@deokjinkim
Copy link
Contributor Author

deokjinkim commented Dec 21, 2022 •
edited
Loading

@lpinca I understand your intention. Then how about adding invalid hostname as last one like below? If you confirm, I'll update commit.

const myURL = new URL('https://example.org:81/foo');
console.log(myURL.hostname);
// Prints example.org

// Setting the hostname does not change the port
myURL.hostname = 'example.com';
console.log(myURL.href);
// Prints https://example.com:81/foo

// Use myURL.host to change the hostname and port
myURL.host = 'example.org:82';
console.log(myURL.href);
// Prints https://example.org:82/foo

// Setting an invalid hostname is ignored
myURL.hostname = 'example.com:81';
console.log(myURL.href);
// Prints https://example.org:82/foo

@lpinca
Copy link
Member

lpinca commented Dec 22, 2022

@deokjinkim why? It's already written on line 265

Invalid host name values assigned to the hostname property are ignored.

The only change required is myURL.hostname = 'example.com:82'; -> myURL.hostname = 'example.com';. Everything else is correct as is.

@deokjinkim
Copy link
Contributor Author

@deokjinkim why? It's already written on line 265

Invalid host name values assigned to the hostname property are ignored.

The only change required is myURL.hostname = 'example.com:82'; -> myURL.hostname = 'example.com';. Everything else is correct as is.

@lpinca I thought wrong use-case can help user. But applied your suggestion because your suggestion is reasonable.

@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 23, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/45927
✔  Done loading data for nodejs/node/pull/45927
----------------------------------- PR info ------------------------------------
Title      doc: fix wrong output of example in `url.hostname` (#45927)
Author     Deokjin Kim  (@deokjinkim)
Branch     deokjinkim:221221_fix_wrong_output_url -> nodejs:main
Labels     doc, url, author ready, commit-queue-squash
Commits    4
 - doc: fix wrong output of example in `url.hostname`
 - Update doc/api/url.md
 - Change hostname using host
 - Address reviewer's comment
Committers 2
 - Deokjin Kim 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/45927
Reviewed-By: Antoine du Hamel 
Reviewed-By: Mohammed Keyvanzadeh 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/45927
Reviewed-By: Antoine du Hamel 
Reviewed-By: Mohammed Keyvanzadeh 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - Address reviewer's comment
   ℹ  This PR was created on Wed, 21 Dec 2022 02:11:53 GMT
   ✔  Approvals: 2
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/45927#pullrequestreview-1225856058
   ✔  - Mohammed Keyvanzadeh (@VoltrexKeyva): https://github.com/nodejs/node/pull/45927#pullrequestreview-1226411171
   ✔  Last GitHub CI successful
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/3762762798

@lpinca
Copy link
Member

lpinca commented Dec 23, 2022 •
edited
Loading

@deokjinkim would you be so kind to squash the commits and update the final commit message to be consistent with the change? Thank you.

@deokjinkim
doc: remove port from example in url.hostname
6ece8fd 
If port is used for `url.hostname`, `url.hostname`
is not working. So remove port from example in
`url.hostname`.
@deokjinkim deokjinkim changed the title doc: fix wrong output of example in url.hostname doc: remove port from example in url.hostname Dec 24, 2022
@deokjinkim
Copy link
Contributor Author

@deokjinkim would you be so kind to squash the commits and update the final commit message to be consistent with the change? Thank you.

@lpinca I squashed commits. Thank your for your review and guide. :)

@lpinca lpinca added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Dec 24, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 24, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/45927
✔  Done loading data for nodejs/node/pull/45927
----------------------------------- PR info ------------------------------------
Title      doc: remove port from example in `url.hostname` (#45927)
Author     Deokjin Kim  (@deokjinkim)
Branch     deokjinkim:221221_fix_wrong_output_url -> nodejs:main
Labels     doc, url, author ready, commit-queue-failed
Commits    1
 - doc: remove port from example in `url.hostname`
Committers 1
 - Deokjin Kim 
PR-URL: https://github.com/nodejs/node/pull/45927
Reviewed-By: Antoine du Hamel 
Reviewed-By: Mohammed Keyvanzadeh 
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/45927
Reviewed-By: Antoine du Hamel 
Reviewed-By: Mohammed Keyvanzadeh 
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - doc: remove port from example in `url.hostname`
   ℹ  This PR was created on Wed, 21 Dec 2022 02:11:53 GMT
   ✔  Approvals: 3
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/45927#pullrequestreview-1225856058
   ✔  - Mohammed Keyvanzadeh (@VoltrexKeyva): https://github.com/nodejs/node/pull/45927#pullrequestreview-1226411171
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/45927#pullrequestreview-1228759740
   ✔  Last GitHub CI successful
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/3770111597

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 24, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 24, 2022
@nodejs-github-bot nodejs-github-bot merged commit 2ad3b02 into nodejs:main Dec 24, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 2ad3b02

targos pushed a commit that referenced this pull request Jan 1, 2023
@deokjinkim @targos
doc: remove port from example in url.hostname
73640eb 
If port is used for `url.hostname`, `url.hostname`
is not working. So remove port from example in
`url.hostname`.

PR-URL: #45927
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2023
Merged
RafaelGSS pushed a commit that referenced this pull request Jan 4, 2023
@deokjinkim @RafaelGSS
doc: remove port from example in url.hostname
d3d808a 
If port is used for `url.hostname`, `url.hostname`
is not working. So remove port from example in
`url.hostname`.

PR-URL: #45927
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Jan 5, 2023
@deokjinkim @RafaelGSS
doc: remove port from example in url.hostname
4457e05 
If port is used for `url.hostname`, `url.hostname`
is not working. So remove port from example in
`url.hostname`.

PR-URL: #45927
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
@deokjinkim @juanarbol
doc: remove port from example in url.hostname
85202f5 
If port is used for `url.hostname`, `url.hostname`
is not working. So remove port from example in
`url.hostname`.

PR-URL: #45927
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
@deokjinkim @juanarbol
doc: remove port from example in url.hostname
83875f4 
If port is used for `url.hostname`, `url.hostname`
is not working. So remove port from example in
`url.hostname`.

PR-URL: #45927
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VoltrexKeyva VoltrexKeyva VoltrexKeyva approved these changes

@aduh95 aduh95 aduh95 approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. doc Issues and PRs related to the documentations. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@deokjinkim @lpinca @nodejs-github-bot @aduh95 @VoltrexKeyva