url: fix typo in comment #2071
url: fix typo in comment #2071
Conversation
|
LGTM |
|
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 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. |
|
Agree this should land. The changed comment isn't from a normal sentence with context. It's specific and has precise interpretation. LGTM |
|
@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. |
|
Pile-on LGTM, the comment is factually wrong. Apropos deletions, I'm okay with those, they don't show up in |
Right, I did know that bit. :) LGTM also then. Carry on. |
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>
|
Merged in 6c61ca5 |
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>
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)