FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: remove port from example in `url.hostname` by deokjinkim · Pull Request #45927 · nodejs/node · GitHub

/ node Public

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

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
deokjinkim:221221_fix_wrong_output_url
Dec 24, 2022
ConversationCommits (1)ChecksFiles changed
Merged

doc: remove port from example in url.hostname#45927
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
deokjinkim:221221_fix_wrong_output_url

Conversation

deokjinkim commented Dec 21, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

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 reviewed Dec 21, 2022
View reviewed changes
Comment thread doc/api/url.md Outdated
aduh95 reviewed Dec 21, 2022
View reviewed changes
Comment thread doc/api/url.md Outdated
aduh95 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 21, 2022
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 reviewed Dec 21, 2022
View reviewed changes
Comment thread doc/api/url.md Outdated

lpinca Dec 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
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.

Comment thread doc/api/url.md Outdated

lpinca Dec 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
myURL.hostname = 'example.com:82';
myURL.hostname = 'example.com';

Comment thread doc/api/url.md Outdated

lpinca Dec 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
// Prints https://example.org:81/foo
// Prints https://example.com:81/foo

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

lpinca Dec 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
myURL.host = 'example.com:82';
myURL.host = 'example.org:82';

Comment thread doc/api/url.md Outdated

lpinca Dec 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
// Prints https://example.com:82/foo
// Prints https://example.org:82/foo

deokjinkim commented Dec 21, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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 commented Dec 22, 2022

Copy link
Copy Markdown
Member

@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 commented Dec 23, 2022

Copy link
Copy Markdown
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 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 commented Dec 23, 2022

Copy link
Copy Markdown
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 commented Dec 23, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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

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 force-pushed the 221221_fix_wrong_output_url branch from 3c5ae76 to 6ece8fd Compare December 24, 2022 00:33
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 commented Dec 24, 2022

Copy link
Copy Markdown
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 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 removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 24, 2022

nodejs-github-bot commented Dec 24, 2022

Copy link
Copy Markdown
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 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 24, 2022
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 merged commit 2ad3b02 into nodejs:main Dec 24, 2022

nodejs-github-bot commented Dec 24, 2022

Copy link
Copy Markdown
Collaborator

Landed in 2ad3b02

targos pushed a commit that referenced this pull request Jan 1, 2023
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 mentioned this pull request Jan 2, 2023
Merged
RafaelGSS pushed a commit that referenced this pull request Jan 4, 2023
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
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
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 mentioned this pull request Jan 28, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lpinca lpinca approved these changes
aduh95 aduh95 approved these changes
+1 more reviewer
VoltrexKeyva VoltrexKeyva approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL