-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
doc: add CTC members to Collaborators list #13284
Conversation
/cc @evanlucas in case this would adversely affect any of his tooling. (I'm guessing it actually would simplify things.) |
Looks good, but just to ask … have you considered moving the CTC blocks as they are, just increasing the heading level by 1? That would also make clear who’s a collaborator and has less duplication (which I guess doesn’t matter much but a lot of our brains are wired to avoid that when it comes to code 😄). |
I'm sorry, I don't understand the suggestion here. Can you re-word it or show a short example? |
I’m basically suggesting to rename |
Ah, I see now! My main concern is that as the Collaborators list gets longer and longer, it's less and less obvious (when you're scanning the middle of the list) that if you don't find someone, there's another list to look at. So I'd prefer to keep CTC folks listed in the Collaborators list. Basically, if the list is any longer than a typical screen, I think it's best if it's a list that contains everyone. As you scroll, it becomes less obvious that some people aren't listed in the expected place. So just keep it simple and list everyone in their expected place. That's my thinking, anyway. |
With the current structure and role of the CTC being what it is, I'm not a fan of this. I'd much prefer @addaleax's proposed approach. |
I don't understand how this change relates to CTC structure and role. Can you elaborate? (Uh, when you get a chance, that is. I know the next 24 hours or so are pretty busy for you...) |
Right now, it's rather important for it to be pretty clear who the CTC members are. I know this still maintains the separate listing, but then we're just duplicating ourselves which makes this change significantly less valuable. |
Is there any reason the CTC members list has to be in |
The visibility of the list makes it far easier to know who needs to sign off on semver-major PRs since those require at least two CTC members to sign off. |
That said, I'd be happy with an approach that eliminates duplication but simplifies the current listing. Like perhaps simply flagging the CTC members in the collaborators list somehow (specific icon perhaps?) |
It might make sense to move the CTC members to to top of the list in addition to the icon ? |
-1 on moving to the top. I don't think there is anything here about removing the CTC list above |
I don't understand how duplication makes the change less valuable. I'm bewildered by the desire to avoid duplication of names in two separate lists. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear, I am good with this change! Just mentioned what I thought I’d find more intuitive but this makes sense to me either way.
ping @nodejs/collaborators |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The duplication isn't ideal, but it's better than being amibiguous.
I think it might be worth adding a LGTM as it is as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
README.md
Outdated
**Trevor Norris** <[email protected]> | ||
* [Trott](https://github.com/Trott) - | ||
**Rich Trott** <[email protected]> (he/him) | ||
>>>>>>> doc: add CTC members to Collaborators list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge conflict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops. Fixed.
81a6ff9
to
0a95767
Compare
For simplicity and clarity (if not brevity), add CTC and CTC Emeriti to Collaborators list in README. This will avoid confusion about who is and isn't a Collaborator.
Landed in bec3877 |
For simplicity and clarity (if not brevity), add CTC and CTC Emeriti to Collaborators list in README. This will avoid confusion about who is and isn't a Collaborator. PR-URL: nodejs#13284 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
For simplicity and clarity (if not brevity), add CTC and CTC Emeriti to Collaborators list in README. This will avoid confusion about who is and isn't a Collaborator. PR-URL: nodejs#13284 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
For simplicity and clarity (if not brevity), add CTC and CTC Emeriti to Collaborators list in README. This will avoid confusion about who is and isn't a Collaborator. PR-URL: #13284 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
For simplicity and clarity (if not brevity), add CTC and CTC Emeriti to Collaborators list in README. This will avoid confusion about who is and isn't a Collaborator. PR-URL: #13284 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
For simplicity and clarity (if not brevity), add CTC and CTC Emeriti to Collaborators list in README. This will avoid confusion about who is and isn't a Collaborator. PR-URL: #13284 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
For simplicity and clarity (if not brevity), add CTC and CTC Emeriti to Collaborators list in README. This will avoid confusion about who is and isn't a Collaborator. PR-URL: #13284 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
For simplicity and clarity (if not brevity), add CTC and CTC Emeriti to Collaborators list in README. This will avoid confusion about who is and isn't a Collaborator. PR-URL: #13284 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
For simplicity and clarity (if not brevity), add CTC and CTC Emeriti to Collaborators list in README. This will avoid confusion about who is and isn't a Collaborator. PR-URL: #13284 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
For simplicity and clarity (if not brevity), add CTC and CTC Emeriti to
Collaborators list in README. This will avoid confusion about who is and
isn't a Collaborator.
Checklist
Affected core subsystem(s)
doc