-
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: small makeover for onboarding.md #13413
Conversation
doc/onboarding.md
Outdated
* You may not `LGTM` your own pull requests. | ||
* You have the power to `LGTM` anyone else's pull requests. | ||
the changes in a pull request using Github’s approval interface | ||
* Some people like to comment `LGTM` (“Looks Good To Me”) |
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.
Do we usually use non-ASCII characters as quotation marks? There are ASCII quotation marks in lines 69 and 77.
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 to do this, mostly because it does look a lot nicer to me (and partly because I like to know if something breaks because of this – if something can’t handle UTF-8 in 2017, it’s useless). I’ve done this before in the docs, and so far nobody has complained.
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.
That's fine by me, just wanted to check :)
doc/onboarding.md
Outdated
* if node itself needs it (due to historic reasons), then it belongs in node | ||
* that is to say, url is there because of http, freelist is there because of http, et al | ||
* also, things that cannot be done outside of core, or only with significant pain (example: async-wrap) | ||
* Opinions vary – it’s good to have a broad collaborator base for that reason! |
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.
Same here, Unicode while line 112 still sticks to ASCII. Please feel free to ignore this if you feel it is irrelevant :)
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.
+1, this should be consistent
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.
fwiw I don’t see the while line 112 still sticks to ASCII
one, are you sure you got the right line?
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.
Line 112: + * You have the power to approve any other collaborator's work.
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 think they mean new line 112 - https://github.com/nodejs/node/pull/13413/files#diff-c00e7ffdc40896f8ff6567c07f9eb43cR112
@@ -32,33 +33,38 @@ onboarding session. | |||
## Local setup | |||
|
|||
* git: | |||
* make sure you have whitespace=fix: `git config --global --add apply.whitespace fix` | |||
* usually PR from your own github fork | |||
* Make sure you have whitespace=fix: `git config --global --add apply.whitespace fix` |
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.
Maybe it would be better to set this only for the local node repo?
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’ve been mentioning manually sometimes that you can set this locally if you really want to… I know I would like to have this as a global option, but sure, if others don’t, I can change this.
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'm good with this as written
* make sure you have whitespace=fix: `git config --global --add apply.whitespace fix` | ||
* usually PR from your own github fork | ||
* Make sure you have whitespace=fix: `git config --global --add apply.whitespace fix` | ||
* Always continue to PR from your own github fork |
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 don't think this is universally true - sometimes there is value in having a PR off a branch that others can easily edit.
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.
We have Allow edits from maintainers.
for that now, right?
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.
With the ability for collaborators to push into PR branches in forks, is that really an issue tho?
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.
There is an edge case. If the PR is against the fork's master
you can't force push a rebased ref set, or if you can you lose the ability to edit.
So maybe:
Continue to always create PRs from branches on your own github fork
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.
There is an edge case. If the PR is against the fork's
master
you can't force push a rebased ref set, or if you can you lose the ability to edit.
Do you mean PR from a fork’s master
? I don’t think we need to worry about that, virtually all collaborators have made more than one PR at the time of their onboarding so they know not to do that.
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'm good anyway
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.
+1 to this, using branches in Node is not usual behaviour, we should start with the standard workflow.
Maybe:
- * Always continue to PR from your own github fork
+ * Continue to raise PRs from your own github fork
though
doc/onboarding.md
Outdated
|
||
|
||
## Project goals & values | ||
|
||
* collaborators are effectively part owners | ||
* the project has the goals of its contributors | ||
* Collaborators are effectively the owners of the project |
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 collective
?
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.
done!
doc/onboarding.md
Outdated
* You may not `LGTM` your own pull requests. | ||
* You have the power to `LGTM` anyone else's pull requests. | ||
the changes in a pull request using Github’s approval interface | ||
* Some people like to comment `LGTM` (“Looks Good To Me”) |
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.
Maybe we should have an acronyms section? LGTM, PTAL, etc
139f613
to
a7ea0ba
Compare
doc/onboarding.md
Outdated
* You have the power to `LGTM` anyone else's pull requests. | ||
the changes in a pull request using Github’s approval interface | ||
* Some people like to comment `LGTM` (“Looks Good To Me”) | ||
* You have the power to approve any another collaborator's work. |
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.
any another
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.
done!
a7ea0ba
to
455512e
Compare
doc/onboarding.md
Outdated
* [https://github.com/nodejs/NG](https://github.com/nodejs/NG) | ||
* [https://github.com/nodejs/api](https://github.com/nodejs/api) | ||
* Don't worry about making mistakes: everybody makes them, there's a lot to internalize and that takes time (and we recognize that!) | ||
* No mistakes are unrecoverable! |
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.
except certain operations on GitHub comments 😞
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 have no suggestions on how to say this succinctly... but it may be worth adding a reminder that deleting the branch for a PR removes the commit history from that PR (that is, the PR will show 0 commits / 0 changes)
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.
Turn it into a positive:
Almost any mistake you could make can be fixed or reverted
IMHO it's good to add but try to learn from your mistakes
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 nits and suggestions.
doc/onboarding.md
Outdated
* generally: try to be nice to people | ||
* There are some higher-level goals and values | ||
* Not everything belongs in core (if it can be done reasonably in userland, let it stay in userland) | ||
* Empathy towards users matters (this is in part why we onboard people) |
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.
IMHO empathy is too vague:
Maybe Being kind and courteous towards users is important
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.
@refack I think "being kind and courteous" is too low a bar. I agree that "empathy" can be hard to define specifically and may not be understood by everyone, but I prefer it to "be nice" which (to me, at least) minimizes things.
Maybe what's needed here is a link on the word "empathy" to another doc (or another part of this doc) that might explain things more fully. Probably out-of-scope for this PR, though. And I'm certainly open to other ideas.
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.
(Also, "be nice to people" is spelled out in the next line, woot!)
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'm happy if we can assume that everyone watched "Star Trek TNG" and knows what an Empath is 😄
doc/onboarding.md
Outdated
|
||
* `#node-dev` on [webchat.freenode.net](https://webchat.freenode.net/) is the best place to interact with the CTC / other collaborators | ||
* If there are any questions after the session, this is a good place to ask! |
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.
Nit (no so sure)
maybe /s/this/there/
or ..., a good place to ask is there.
doc/onboarding.md
Outdated
* You have the power to `LGTM` anyone else's pull requests. | ||
the changes in a pull request using Github’s approval interface | ||
* Some people like to comment `LGTM` (“Looks Good To Me”) | ||
* You have the power to approve any other collaborator's work. |
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.
maybe /s/power/authority/
doc/onboarding.md
Outdated
the changes in a pull request using Github’s approval interface | ||
* Some people like to comment `LGTM` (“Looks Good To Me”) | ||
* You have the power to approve any other collaborator's work. | ||
* You may not approve your own pull requests. |
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't (GitHub does not allow it) so:
You can't approve your own pull requests, so you'll need at least another Collaborator's approval for you own PRs.
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’ll pick the first part of the suggestion, the rest is kind of implicit, right? (Also, remember that there’s usually an extra audio or text stream that goes over all of this – I’m okay with not explaining everything in here, just the outlines)
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 was just imagining someone reading this beforehand, and it was a one of questions I explicitly asked.
doc/onboarding.md
Outdated
* Some people like to comment `LGTM` (“Looks Good To Me”) | ||
* You have the power to approve any other collaborator's work. | ||
* You may not approve your own pull requests. | ||
* When explicitly using `Changes requested`, show empathy – comments will |
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.
Re nodejs/CTC#128 I would elaborate on it "blocking":
When explicitly using `Changes requested` you are effectively blocking the PR, so be courteous and explain why you consider your comment a "blocking issue" - comment...
doc/onboarding.md
Outdated
* if node itself needs it (due to historic reasons), then it belongs in node | ||
* that is to say, url is there because of http, freelist is there because of http, et al | ||
* also, things that cannot be done outside of core, or only with significant pain (example: async-wrap) | ||
* Opinions vary – it’s good to have a broad collaborator base for that reason! |
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 think they mean new line 112 - https://github.com/nodejs/node/pull/13413/files#diff-c00e7ffdc40896f8ff6567c07f9eb43cR112
doc/onboarding.md
Outdated
* also, things that cannot be done outside of core, or only with significant pain (example: async-wrap) | ||
* Opinions vary – it’s good to have a broad collaborator base for that reason! | ||
* If node itself needs it (due to historic reasons), then it belongs in node | ||
* That is to say, url is there because of http, freelist is there because of http, et al |
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.
period after et al.
(it's in the phrase not an end of sentence period)
or even replace with the more common etc.
(also .
is part of the word)
doc/onboarding.md
Outdated
* Opinions vary – it’s good to have a broad collaborator base for that reason! | ||
* If node itself needs it (due to historic reasons), then it belongs in node | ||
* That is to say, url is there because of http, freelist is there because of http, et al | ||
* Also, things that cannot be done outside of core, or only with significant pain (for example `async_hooks`) |
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.
IMHO since it's a list Also,
is unnecessary.
doc/onboarding.md
Outdated
* [https://github.com/nodejs/NG](https://github.com/nodejs/NG) | ||
* [https://github.com/nodejs/api](https://github.com/nodejs/api) | ||
* Don't worry about making mistakes: everybody makes them, there's a lot to internalize and that takes time (and we recognize that!) | ||
* No mistakes are unrecoverable! |
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.
Turn it into a positive:
Almost any mistake you could make can be fixed or reverted
IMHO it's good to add but try to learn from your mistakes
@refack I’ve addressed most of your review (where I think it made sense – thanks for the suggestions!), PTAL |
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.
💯
doc/onboarding.md
Outdated
|
||
|
||
## Project goals & values | ||
|
||
* collaborators are effectively part owners | ||
* the project has the goals of its contributors | ||
* Collaborators are effectively the collective owners of the project |
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.
Take-it-or-leave-it nit: While we're in here, let's drop effectively
. It introduces more questions than it answers. Collectively, the Collaborators are the owners of the project. Saying effectively
just signals that we don't actually mean it.
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.
There still is effectively
vs formally
. Isn't the project formerly
owned by the "Node.js Foundation"
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.
BTW: 👍 on removing
doc/onboarding.md
Outdated
* Not everything belongs in core (if it can be done reasonably in userland, let it stay in userland) | ||
* Empathy towards users matters (this is in part why we onboard people) | ||
* Generally: try to be nice to people! | ||
* The best outcome is for people who come to our issue tracker to feel like they can come back again. |
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 this line a lot.
doc/onboarding.md
Outdated
* you have (mostly) free rein – don't hesitate to close an issue if you are confident that it should be closed | ||
* **IMPORTANT**: be nice about closing issues, let people know why, and that issues and PRs can be reopened if necessary | ||
* Still need to follow the Code of Conduct | ||
* You have (mostly) free rein – don't hesitate to close an issue if you are confident that it should be closed |
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.
Micro-nit: Remove the -
and make this two sentences (separated with .
) or two clauses (separated with ;
).
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.
done :) went with ;
, that seemed the most natural to me.
doc/onboarding.md
Outdated
@@ -69,7 +75,8 @@ onboarding session. | |||
* When adding a semver label, add a comment explaining why you're adding it. Do it right away so you don't forget! | |||
|
|||
* [**See "Who to CC in issues"**](./onboarding-extras.md#who-to-cc-in-issues) | |||
* will also come more naturally over time | |||
* For many of the teams listed there, you can just ask to be added if you are interested |
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.
Micro-nit: remove just
Less micro: Remove for many of the teams listed there
. People should freely ask. If they need to do something to join a WG or whatever, then it can be explained when they ask.
doc/onboarding.md
Outdated
* that is to say, url is there because of http, freelist is there because of http, et al | ||
* also, things that cannot be done outside of core, or only with significant pain (example: async-wrap) | ||
* Opinions vary – it’s good to have a broad collaborator base for that reason! | ||
* If node itself needs it (due to historic reasons), then it belongs in node |
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.
Micro-nit node
-> Node.js
in two places on this line
doc/onboarding.md
Outdated
* [https://github.com/nodejs/api](https://github.com/nodejs/api) | ||
* Don't worry about making mistakes: everybody makes them, there's a lot to internalize and that takes time (and we recognize that!) | ||
* Almost any mistake you could make can be fixed or reverted. | ||
* The existing collaborators trust you and are grateful for your help! |
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.
Micro-nit: I think we tend to style it capitalized, so collaborators
-> 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.
LGTM
doc/onboarding.md
Outdated
* [See "Updating Node.js from Upstream"](./onboarding-extras.md#updating-nodejs-from-upstream) | ||
* make new branches for all commits you make! | ||
* Make new branches for all commits you make! |
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.
This suggests that multiple commit PRs are not ok, which I don't think is the intent.
Maybe:
- * Make new branches for all commits you make!
+ * Make a new branch for each PR you raise.
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.
[question to a native english speakers]
raise
vs create
vs request
Intuitively would use raise
for Issue, request
for PR (since PR = "Pull Request", but turned into a noun)? or is create
best?
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.
Not a native English speaker, but my preference would be create
, raise
, request
. Why not submit
?
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 me:
request
sounds weird, you don't request a Pull Request, you request that your changes be landed by raising the Pull Request. It'd be like questioning a question.
I prefer raise
to create
, it has the feeling of bringing it to people's attention, and it matches raising issues. I'd say raise
, create
, submit
, or open
are all fine though.
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 went with submit
, to my ears raise
seems like it matches Issues much better than PRs.
@@ -69,9 +75,11 @@ onboarding session. | |||
* When adding a semver label, add a comment explaining why you're adding it. Do it right away so you don't forget! | |||
|
|||
* [**See "Who to CC in issues"**](./onboarding-extras.md#who-to-cc-in-issues) | |||
* will also come more naturally over time | |||
* This will come more naturally over time | |||
* For many of the teams listed there, you can ask to be added if you are interested |
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.
Maybe also link to https://github.com/orgs/nodejs/teams ? I didn't realise that I could now see who was in what team when I was onboarded.
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’t see that page if you’re not a member, so it’s at least going to be confusing for those who read through the doc a bit in advance.
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.
Looking at this again, I see you linked to the Collaborators and members teams above, so the team listing can probably be deduced for that.
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.
Thanks a lot for doing this @addaleax!
* that is to say, url is there because of http, freelist is there because of http, et al | ||
* also, things that cannot be done outside of core, or only with significant pain (example: async-wrap) | ||
the changes in a pull request using Github’s approval interface | ||
* Some people like to comment `LGTM` (“Looks Good To Me”) |
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 some people like to comment
seems weird to me, the point is that either is fine right?
- the changes in a pull request using Github’s approval interface
- * Some people like to comment `LGTM` (“Looks Good To Me”)
+ the changes in a pull request using Github’s approval interface,
+ or by commenting `LGTM` (“Looks Good To Me”).
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.
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 reason I prefer that method is because github does not automatically remove approvals after new changes are pushed, so having green checkmarks still show after new changes are pushed is not ideal to me. I would rather "risk" having my name left out of the 'reviewed by' list than be erroneously listed as having approved something that I didn't review.
I understand it's ultimately inevitable at the time of landing, as metadata is typically landed last, but I'm mostly concerned about pushes prior to that point.
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.
@mscdex interesting point, but I'm not sure how commenting LGTM is better. If people are using the node-review
chrome extension, that'll add your name to the metadata anyway, no matter when you LGTMed.
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.
Sounds like it would be easier to fix in node-review
then get that as a feature from GitHub (dismiss approvals on new commits)
On second thought it's not trivial, since new commits could be just nit fixes or benign improvements 🤔
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.
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.
It's mostly for anyone who solely uses the "approved" list for the "reviewed by" list. It's better than nothing.
Hmm okay, but doesn't that mean that those people will never record your approval at all?
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.
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.
@gibfahn I already mentioned that I would rather chance not having my name on a PR/commit than have my name listed as having approved something I did not review.
* also, things that cannot be done outside of core, or only with significant pain (example: async-wrap) | ||
the changes in a pull request using Github’s approval interface | ||
* Some people like to comment `LGTM` (“Looks Good To Me”) | ||
* You have the authority to approve any other collaborator’s work. |
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.
Maybe any other collaborator's work
-> any PR
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.
Reasoning: Specifying any other collaborator’s work
suggests that you can't approve non-collaborator's work (at least to me).
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.
maybe anyone else's work
?
doc/onboarding.md
Outdated
* If you do, it is nice if you are available later to check whether your | ||
comments have been addressed | ||
* Other Collaborators will usually take `Changes requested` to mean that | ||
you are considering some of your comments to block the PR from landing. |
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.
Not just other collaborators right? That's exactly what the ❌ means.
- * Other Collaborators will usually take `Changes requested` to mean that
- you are considering some of your comments to block the PR from landing.
+ * `Changes requested` means that you are blocking the PR from landing
+ until the requested changes are made.
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.
Yes, not just collaborators, but those are the ones who decide whether a PR is getting landed (by landing it). I’ve reworded this a bit.
* When explicitly using `Changes requested`, show empathy – comments will | ||
usually be addressed even if you don’t use it. | ||
* If you do, it is nice if you are available later to check whether your | ||
comments have been addressed |
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.
Maybe also be clear that other collaborators will clear your review once the changes have been made.
+ * If you see that the requested changes have been made, you can clear another collaborator's
+ `Changes requested` review.
doc/onboarding.md
Outdated
* empathy towards users matters (this is in part why we onboard people) | ||
* generally: try to be nice to people | ||
* There are some higher-level goals and values | ||
* Not everything belongs in core (if it can be done reasonably in userland, let it stay in userland) |
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 think this is covered below in the What belongs in Node.js:
section.
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.
Ah, yes. Removing this sounds good, it’s always been tripping me up during the onboardings that this is mentioned twice.
b4e9cea
to
b9a776f
Compare
@gibfahn PTAL, and remember that onboarding sessions have an extra audio or text track that explains the items in more detail. :) |
I get that, and tbh this is fine. I think it's a useful document for collaborators to refer to after onboarding as well though. |
* watching the main repo will flood your inbox, so be prepared | ||
* Notifications: | ||
* Use [https://github.com/notifications](https://github.com/notifications) or set up email | ||
* Watching the main repo will flood your inbox (several hundred notifications on typical weekdays), so be prepared |
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 would add:
* It is not required that you watch the nodejs/node repo
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 usually tell people that they have been subscribed to multiple repositories and may want to unsubscribe, and for most existing collaborators I think this is more or less clear.
Landed in 50d1515 |
PR-URL: #13413 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: #13413 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: #13413 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: #13413 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Based on my experience from the last few onboardings.