url: added url.resolve() method's behaviour#8969
url: added url.resolve() method's behaviour#8969minervapanda wants to merge 2 commits intonodejs:masterfrom
Conversation
addaleax
left a comment
There was a problem hiding this comment.
Seems pretty good so far! I’ve left a couple of suggestions, and it would be great if you could update the PR with them. If not, they can also be applied when merging the PR. :)
doc/api/url.md
Outdated
There was a problem hiding this comment.
A couple of suggestions:
- Can you leave a blank line above this one?
- Can you try to wrap the line after 80 characters?
- I’d suggest enclosing the protocol names in backticks, i.e.
https, that seems a bit more readable to me.
doc/api/url.md
Outdated
There was a problem hiding this comment.
I don’t have a strong preference for either way, but probably either all of these lines should end with a semicolon or none of them?
doc/api/url.md
Outdated
There was a problem hiding this comment.
There’s a space missing after Although, (and maybe a blank line above this one, too). Also, it might be good to have an example of that behaviour listed here?
|
The subsystem should be |
|
@minervapanda I think we always use |
|
Yep, what @gibfahn said. |
|
I will take care of this and amend the commit here. Thanks for the clarification. |
3c75eea to
e33fdcd
Compare
doc/api/url.md
Outdated
There was a problem hiding this comment.
oh, one additional minor nit: can you please line wrap at 80-chars?
There was a problem hiding this comment.
Yeah sure. Eliminating this line " Note that url.resolve() method depends on the protocol given in the from parameter." Would be fine right ?
There was a problem hiding this comment.
That first sentence is fine, just add a line break or two in there so a single line doesn't go past 80 chars :-)
558bdd4 to
7fe931c
Compare
|
If I'm not mistaken, it looks like this adds four files that shouldn't be there. This should just be the changes to |
|
Tiny nit: The verb ( |
|
Yes. @Trott Everything was okay in the previous commit.While committing again , by mistake I changed 4 other files. I am fixing this now. I am puzzled , what are those other files. |
873871b to
c0e1785
Compare
|
Yeah, this looks good. |
|
@minervapanda Any specific reason to close this? You may want to remove the last commit here but otherwise this PR was just fine. |
|
|
|
@minervapanda use a different branch for each issue/PR. From your fork of node run this for each PR you want to submit: Do your changes, then commit and send a PR from that branch. |
Checklist
Affected core subsystem(s)
url
Description of change
Solved #8921 - Updated documentation for url.resolve() by mentioning about how the method behaves differently with the change of protocol in the "from" parameter.