-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: improve onboarding extras formatting #6548
doc: improve onboarding extras formatting #6548
Conversation
Two stray commits in here? |
| `test/*` | @nodejs/testing, @trott | | ||
| `tools/eslint`, `.eslintrc` | @silverwind, @trott | | ||
| upgrading v8 | @bnoordhuis, @targos, @ofrobots | | ||
| upgrading npm | @thealphanerd, @fishrock123 | |
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.
Would much prefer to make these all team references and begin moving away from specific individuals. Doing so has worked effectively for things like lts, documentation, streams, http, and crypto. For instance, I've created a @nodejs/buffer team as an example.
The downside to mentioning specific individuals is that it creates a disincentive for new collaborators to feel empowered to jump in and help with the reviews and puts too much emphasis and burden on specific individuals.
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.
For some areas, yes.
For other parts of core, I think this is still correct until the knowledge becomes more distilled and distributed as more people learn the codebase, and while that is something we should work towards it doesn't necessarily reflect reality.
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.
Having those teams be initially made up of the people listed here would meet the same objective, no?
Also, this should also list:
doc
- @nodejs/documentation- anything LTS - @nodejs/lts
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.
@jasnell Will add those, could you come up with a better LTS description?
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.
I like the team approach, but do also see your pov @Fishrock123. I think we should definitely try to move towards having a team for each subsystem eventually.
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.
You can remove my name from testing as I'm on nodejs/testing and that's what people should be using IMO.
No, but I did have a stray commit in master which is why the base is now incorrect here, fixing. |
f50d7b8
to
0111f0f
Compare
This was mostly to fix the formatting.. can we land this and then PR any additions after? I'm accepting, and had already previously accepted the list was not complete and we should probably get the collaborators to to fill it out more. It's designed to be a baseline guide for those new to the project to be able to CC at least some people. :) Also, perhaps it should be in one of the other documents, I'm not quite sure! |
LGTM |
No objections, just want to make sure it's not left undone. |
0111f0f
to
f643e94
Compare
fffff this has that stupid |
f643e94
to
02a2926
Compare
@jasnell lgty? |
Looks fine. |
LGTM |
Fixes some formatting, improves some formatting, updates minor nits. Refs: nodejs#6655 PR-URL: nodejs#6548 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
02a2926
to
2fee506
Compare
FWIW explicitly mentioning me for |
Checklist
Affected core subsystem(s)
doc,meta
Description of change
Fixes some formatting, improves some formatting, updates minor nits.
I guess we should at some point do a call for who wants to be on the chart.