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

doc: mention cc-ing nodejs/url team for reviews by addaleax · Pull Request #10652 · nodejs/node · GitHub

/ node Public

doc: mention cc-ing nodejs/url team for reviews - #10652

Closed
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:doc-cc-nodejs-url
ConversationCommits (1)ChecksFiles changed
Closed

doc: mention cc-ing nodejs/url team for reviews#10652
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:doc-cc-nodejs-url

Conversation

addaleax commented Jan 6, 2017

Copy link
Copy Markdown
Member

Add the nodejs/url github team to the table of people to /cc for
reviews on the WHATWG URL code.

fyi @nodejs/url

nodejs-github-bot added doc Issues and PRs related to the documentations. lts-watch-v6.x labels Jan 6, 2017
addaleax added meta Issues and PRs related to the general management of the project. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jan 6, 2017
TimothyGu reviewed Jan 6, 2017
View reviewed changes
Comment thread doc/onboarding-extras.md Outdated

TimothyGu Jan 6, 2017 •
edited
Loading

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

nit: None Pretty much none of the other entries in the file seem to have .js

addaleax Jan 6, 2017

Copy link
Copy Markdown
Member Author

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

@TimothyGu thanks for noticing, fixed!

Trott reviewed Jan 6, 2017
View reviewed changes
Comment thread doc/onboarding-extras.md Outdated

Trott Jan 6, 2017 •
edited
Loading

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

Should it be lib/url? internal seems to be implied everywhere else (e.g., lib/fs would presumably include lib/internal/fs.js).

mscdex Jan 6, 2017

Copy link
Copy Markdown
Contributor

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

I think it's about difference in implementations. WHATWG vs original url.

jasnell Jan 6, 2017

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

yep... lib/internal/url.js is the home of the new implementation. Intentionally put there to keep it distinct from the older lib/url.js

Trott Jan 6, 2017

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

That's going to be confusing/unclear if there's not a separate entry for lib/url in addition to the entry for lib/internal/url. Not opposed to this landing as is, but if there's different people to cc depending on which one you're working on, I'd prefer that there be entries for both.

joyeecheung Jan 6, 2017

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

Maybe adding a note column and mention that lib/url doesn't apply to this would help?

addaleax Jan 10, 2017

Copy link
Copy Markdown
Member Author

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

I don’t think there is anybody in particular to ping for lib/url, and I think leaving this as-is is going to be fine. If it turns out to be a horrible idea, we can add a note or find people to @mention for lib/url.

joyeecheung requested changes Jan 6, 2017
View reviewed changes
Comment thread doc/onboarding-extras.md Outdated

joyeecheung Jan 6, 2017 •
edited
Loading

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

There is src/node_url.cc too.

EDIT: should be src/node_url, there is also a .h.

jasnell commented Jan 10, 2017

Copy link
Copy Markdown
Member

ping @addaleax

doc: mention cc-ing nodejs/url team for reviews
09590d0 
Add the nodejs/url github team to the table of people to /cc for
reviews on the WHATWG URL code.

addaleax commented Jan 10, 2017

Copy link
Copy Markdown
Member Author

Sorry, missed @joyeecheung’s review. updated!

jasnell commented Jan 10, 2017

Copy link
Copy Markdown
Member

@joyeecheung ... does this look good to you now?

jasnell pushed a commit that referenced this pull request Jan 11, 2017
doc: mention cc-ing nodejs/url team for reviews
fa38361 
Add the nodejs/url github team to the table of people to /cc for
reviews on the WHATWG URL code.

PR-URL: #10652
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

jasnell commented Jan 11, 2017

Copy link
Copy Markdown
Member

Landed in fa38361

jasnell closed this Jan 11, 2017
addaleax deleted the doc-cc-nodejs-url branch January 12, 2017 18:01
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
doc: mention cc-ing nodejs/url team for reviews
3e29272 
Add the nodejs/url github team to the table of people to /cc for
reviews on the WHATWG URL code.

PR-URL: nodejs#10652
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017
doc: mention cc-ing nodejs/url team for reviews
23f85ab 
Add the nodejs/url github team to the table of people to /cc for
reviews on the WHATWG URL code.

PR-URL: nodejs#10652
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 24, 2017
doc: mention cc-ing nodejs/url team for reviews
63df2b6 
Add the nodejs/url github team to the table of people to /cc for
reviews on the WHATWG URL code.

PR-URL: nodejs#10652
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
italoacasas mentioned this pull request Jan 25, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
doc: mention cc-ing nodejs/url team for reviews
56ea7eb 
Add the nodejs/url github team to the table of people to /cc for
reviews on the WHATWG URL code.

PR-URL: nodejs#10652
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
italoacasas mentioned this pull request Jan 29, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Mar 7, 2017
doc: mention cc-ing nodejs/url team for reviews
005f2b3 
Add the nodejs/url github team to the table of people to /cc for
reviews on the WHATWG URL code.

PR-URL: #10652
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
doc: mention cc-ing nodejs/url team for reviews
8e807f6 
Add the nodejs/url github team to the table of people to /cc for
reviews on the WHATWG URL code.

PR-URL: #10652
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins mentioned this pull request Mar 9, 2017
Merged
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

Trott Trott left review comments
jasnell jasnell approved these changes
joyeecheung joyeecheung approved these changes
+4 more reviewers
mscdex mscdex left review comments
TimothyGu TimothyGu approved these changes
Fishrock123 Fishrock123 approved these changes
gibfahn gibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

Back | FazBrowse Home | New Git URL