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.

Sign up for GitHub

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

Jump to bottom

doc: add the diagnostics team to cc for async_wrap #9471

Closed
wants to merge 1 commit into from
Closed

doc: add the diagnostics team to cc for async_wrap #9471

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Nov 4, 2016

Add the nodejs/diagnostics team to the “Who to CC in issues” list
as the maintainers of async_hooks.

Ref: #9467 (comment)

@addaleax addaleax added the meta Issues and PRs related to the general management of the project. label Nov 4, 2016
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 4, 2016
@addaleax addaleax mentioned this pull request Nov 4, 2016
Closed
@sam-github
Copy link
Contributor

LGTM

@addaleax
Copy link
Member Author

addaleax commented Nov 4, 2016

/cc @nodejs/diagnostics

@Qard
Copy link
Member

Qard commented Nov 4, 2016

LGTM also

@joshgav
Copy link
Contributor

joshgav commented Nov 4, 2016

LGTM

/cc @trevnorris @AndreasMadsen

@Fishrock123
Copy link
Contributor

I feel like that group has far more people than actually know in detail about it, maybe a separate team is more appropriate? (Also I guess I should be joining the Diagnostics WG...)

@AndreasMadsen
Copy link
Member

I feel like that group has far more people than actually know in detail about it, maybe a separate team is more appropriate?

Agreed. But isn't the list more about who to cc for issues, than who to cc for PR reviews? There are a lot of people in @nodejs/diagnostics who are async_hooks stakeholders, without necessarily being async_hooks experts.

@Fishrock123
Copy link
Contributor

@AndreasMadsen That does expose some ambiguity, but I originally made this with two things in mind:

  • PR reviews
  • Review for deep issues (bugs etc)

General discussion is imo a bit more high level and idk where to put that exactly.

@AndreasMadsen
Copy link
Member

@Fishrock123 I see. If the list is for PR reviews and similar then @nodejs/diagnostics is definitely too board.

@addaleax
Copy link
Member Author

addaleax commented Nov 4, 2016

@AndreasMadsen so it is just you and @trevnorris then?

cjihrig
cjihrig approved these changes Nov 5, 2016
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

This is something that we need, so LGTM once it's decided exactly who should be CCed :-)

@Trott
Copy link
Member

Trott commented Nov 5, 2016

Nit: Except for the platform wildcard entry at the bottom, everything else in the list is in alphabetical (or at least alphabetical-ish) order, so perhaps this new entry should be placed alphabetically as well?

@AndreasMadsen
Copy link
Member

@addaleax I think there are others. Maybe @Fishrock123 and @Qard?

@Fishrock123
Copy link
Contributor

I think @trevnorris, @AndreasMadsen, and myself are probably the ones who best understand the implementation right now.

@addaleax addaleax force-pushed the who-to-cc-in-issues-async_wrap branch from 15ba013 to b6adbd3 Compare November 6, 2016 13:54
@addaleax
Copy link
Member Author

addaleax commented Nov 6, 2016

I’ve updated this with @trevnorris, @AndreasMadsen, @Fishrock123, @Qard and tried to address @Trott’s nit (which may have failed because I am not sure what exactly is supposed to be ordered alphabetically here)

@Qard
Copy link
Member

Qard commented Nov 20, 2016

Does anyone in @nodejs/diagnostics specifically object to getting notifications about async_wrap stuff? I mean, even if they aren't experts on the subject, I feel like basically everyone in the working group would have some interest in at least being notified of the developments relating to async_wrap.

@watson
Copy link
Member

watson commented Nov 21, 2016

@Qard I'd love to get notifications on this

@addaleax
doc: add people to cc for async_wrap
d93c0f8 
Add a group of people to the “Who to CC in issues” list
as the maintainers of `async_hooks`.

Ref: nodejs#9467 (comment)
@addaleax addaleax force-pushed the who-to-cc-in-issues-async_wrap branch from b6adbd3 to d93c0f8 Compare November 26, 2016 06:55
@addaleax
Copy link
Member Author

@Qard Okay, I’ve switched this back to @nodejs/diagnostics. I guess we can still update the doc to be more selective if it turns out that this is too noisy.

@addaleax
Copy link
Member Author

addaleax commented Dec 5, 2016

Landed in 62eee49

@addaleax addaleax closed this Dec 5, 2016
@addaleax addaleax deleted the who-to-cc-in-issues-async_wrap branch December 5, 2016 23:08
addaleax added a commit that referenced this pull request Dec 5, 2016
@addaleax
doc: add people to cc for async_wrap
62eee49 
Add a group of people to the “Who to CC in issues” list
as the maintainers of `async_hooks`.

Ref: #9467 (comment)
PR-URL: #9471
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Josh Gavant <josh.gavant@outlook.com>
@addaleax addaleax mentioned this pull request Dec 5, 2016
Closed
3 tasks
Fishrock123 pushed a commit that referenced this pull request Dec 6, 2016
@addaleax @Fishrock123
doc: add people to cc for async_wrap
51b77aa 
Add a group of people to the “Who to CC in issues” list
as the maintainers of `async_hooks`.

Ref: #9467 (comment)
PR-URL: #9471
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Josh Gavant <josh.gavant@outlook.com>
jmdarling pushed a commit to jmdarling/node that referenced this pull request Dec 8, 2016
@addaleax
doc: add people to cc for async_wrap
bebd0be 
Add a group of people to the “Who to CC in issues” list
as the maintainers of `async_hooks`.

Ref: nodejs#9467 (comment)
PR-URL: nodejs#9471
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Josh Gavant <josh.gavant@outlook.com>
MylesBorins pushed a commit that referenced this pull request Dec 20, 2016
@addaleax @MylesBorins
doc: add people to cc for async_wrap
e8c57bb 
Add a group of people to the “Who to CC in issues” list
as the maintainers of `async_hooks`.

Ref: #9467 (comment)
PR-URL: #9471
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Josh Gavant <josh.gavant@outlook.com>
MylesBorins pushed a commit that referenced this pull request Dec 20, 2016
@addaleax @MylesBorins
doc: add people to cc for async_wrap
176f680 
Add a group of people to the “Who to CC in issues” list
as the maintainers of `async_hooks`.

Ref: #9467 (comment)
PR-URL: #9471
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Josh Gavant <josh.gavant@outlook.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@addaleax @MylesBorins
doc: add people to cc for async_wrap
74980a5 
Add a group of people to the “Who to CC in issues” list
as the maintainers of `async_hooks`.

Ref: #9467 (comment)
PR-URL: #9471
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Josh Gavant <josh.gavant@outlook.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@addaleax @MylesBorins
doc: add people to cc for async_wrap
4dec912 
Add a group of people to the “Who to CC in issues” list
as the maintainers of `async_hooks`.

Ref: #9467 (comment)
PR-URL: #9471
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Josh Gavant <josh.gavant@outlook.com>
This was referenced Dec 21, 2016
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.