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

url: fix typo in comment #2071

Closed
wants to merge 1 commit into from
Closed

url: fix typo in comment #2071

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jun 28, 2015

Checked against results of url.parse() to make sure this was right.

(Not sure about the subsequent comment that indicates this isn't the way Chrome does it. It is, as far as I can tell. Perhaps it's referring to some specific nuance that escapes me. I'll dig through the logs a bit more to see if I can get more information. But if you, kind reader, have an idea, hey, clue me, kthxbai.) (Found the info: 5dc51d4)

@mscdex mscdex added the url Issues and PRs related to the legacy built-in url module. label Jun 28, 2015
@silverwind
Copy link
Contributor

LGTM

@Fishrock123
Copy link
Contributor

I don't really have a formal opinion but I'm not really sure everyone here is too fond of changing code comments without actually having code changes. Reasons include git blame and bisect I believe.

@Fishrock123 Fishrock123 added the doc Issues and PRs related to the documentations. label Jun 30, 2015
@Trott
Copy link
Member Author

Trott commented Jun 30, 2015

@Fishrock123 Fair enough. In this instance at least, the typo causes the comment to provide incorrect information. That incorrect information is especially confounding in conjunction with the subsequent comment that talks about how "this is not the way Chrome does it". (Because, yeah, that's not the way Chrome does it, but it's also not the way Node.js does it! The "it" in this case actually refers to something else.)

I don't usually submit PRs to fix harmless typos in comments, but I have been doing a lot of deletion of obsolete comments. I can refrain from doing that, or maybe I can bundle them all in one mega-comment-cleanup PR instead of doing them one at a time.

@trevnorris
Copy link
Contributor

Agree this should land. The changed comment isn't from a normal sentence with context. It's specific and has precise interpretation.

LGTM

@trevnorris
Copy link
Contributor

@Fishrock123 Though you are correct for the common case. Simple typos in code comments are left out, and only ever changed if surrounding code is also changing.

@bnoordhuis
Copy link
Member

Pile-on LGTM, the comment is factually wrong.

Apropos deletions, I'm okay with those, they don't show up in git blame.

@Fishrock123
Copy link
Contributor

Apropos deletions

Right, I did know that bit. :)

LGTM also then. Carry on.

Trott added a commit that referenced this pull request Jun 30, 2015
PR-URL: #2071
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@Trott
Copy link
Member Author

Trott commented Jun 30, 2015

Merged in 6c61ca5

@Trott Trott closed this Jun 30, 2015
@rvagg rvagg mentioned this pull request Jul 2, 2015
mscdex pushed a commit to mscdex/io.js that referenced this pull request Jul 9, 2015
PR-URL: nodejs#2071
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@Trott Trott deleted the comment-fix branch January 9, 2022 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants