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: added articles about PR communication in CONTRIBUTING.md #17902

Closed
wants to merge 3 commits into from
Closed

doc: added articles about PR communication in CONTRIBUTING.md #17902

wants to merge 3 commits into from

Conversation

SalameWilliam
Copy link

Added some references to PR communication articles in
Helpful Ressources inside CONTRIBUTING.md

Fixes: #16359

Checklist
Affected core subsystem(s)

doc, meta

doc: added articles about PR communication in CONTRIBUTING.md
1fb9fd7 
Added some references to PR communication articles in
Helpful Ressources inside CONTRIBUTING.md

Fixes: nodejs#16359
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Dec 28, 2017
vsemozhetbyt
vsemozhetbyt reviewed Dec 28, 2017
View reviewed changes
CONTRIBUTING.md Outdated
@@ -828,6 +828,8 @@ The following additional resources may be of assistance:
* [How to create a Minimal, Complete, and Verifiable example](https://stackoverflow.com/help/mcve)
* [core-validate-commit](https://github.com/evanlucas/core-validate-commit) -
A utility that ensures commits follow the commit formatting guidelines.
* How to respectfully and usefully review code part [one](https://mtlynch.io/human-code-reviews-1/) and [two](https://mtlynch.io/human-code-reviews-2/)
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Dec 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I may be wrong, but do we need a comma or a colon between "code" and "part" here?

vsemozhetbyt
vsemozhetbyt approved these changes Dec 28, 2017
View reviewed changes
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

Thank you! I have a nit question, but it can be resolved on landing the PR)

@vsemozhetbyt vsemozhetbyt added the meta Issues and PRs related to the general management of the project. label Dec 28, 2017
@Trott
Copy link
Member

Trott commented Dec 28, 2017

Hi, @vixwilli! Welcome, and thanks for the pull request. I have some questions about whether there might be a better place for this information, but I do appreciate the links!

@vsemozhetbyt or anyone else reviewing: Since these linked docs are about reviewing code, don't they more properly belong in the COLLABORATOR_GUIDE.md and not CONTRIBUTING.md?

General outlook from me: Our CONTRIBUTING.md is already waaaaaay too long and I'm reluctant to add anything that isn't absolutely essential. Ref: #17842

@vsemozhetbyt
Copy link
Contributor

@Trott Just a thought: our contributors sometimes also make various brief code notes for each other, so maybe these articles can be helpful for them too. However, I have no strong opinion about the place, so let us hear what others think)

doc: fixed a forgotten comma
670ddfd 
Added a forgotten comma inside CONTRIBUTING.md
@gibfahn
Copy link
Member

gibfahn commented Dec 28, 2017

@vsemozhetbyt or anyone else reviewing: Since these linked docs are about reviewing code, don't they more properly belong in the COLLABORATOR_GUIDE.md and not CONTRIBUTING.md?

If they're primarily for reviewers, then collaborator guide makes more sense IMO.

@starkwang
Copy link
Contributor

+1 on moving this link to the COLLABORATOR_GUIDE.md because the most reviewers of our PR are collaborators.

@SalameWilliam
Copy link
Author

Where do you think I could put the links inside COLLABORATOR_GUIDE.md ? I initially wanted to put it there but since the guide is more about the technical side of things i didn't really know where to insert it. CONTRIBUTING.md already contains a more "human" side of PR like the articles mentionned and a Helpful Ressources section was already present so it was 'easier' to mention it there

doc: moved links about PR communication to COLLABORATOR_GUIDE.md
4ced007 
Moved a few links about PR communication from CONTRIBUTING.md
to COLLABORATOR_GUIDE.md
@SalameWilliam
Copy link
Author

I placed the links inside COLLABORATOR_GUIDE.md in a specific section since the doc is quite long already and I didn't want to clutter the text with them. Tell me if you see a better spot to link them !

@Trott
Copy link
Member

Trott commented Dec 29, 2017

I placed the links inside COLLABORATOR_GUIDE.md in a specific section since the doc is quite long already and I didn't want to clutter the text with them. Tell me if you see a better spot to link them !

I like that location. 👍

@vsemozhetbyt
Copy link
Contributor

CI lite: https://ci.nodejs.org/job/node-test-pull-request-lite/53/

@vsemozhetbyt
Copy link
Contributor

As it appears to be a bit controversial addition, let's wait till Monday before landing, lest anybody have some objections.

vsemozhetbyt pushed a commit that referenced this pull request Jan 2, 2018
@vsemozhetbyt
doc: add references to PR communication articles
d179cca 
Added some references to PR communication articles in
Helpful Ressources inside COLLABORATOR_GUIDE.md

PR-URL: #17902
Fixes: #16359
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
@vsemozhetbyt
Copy link
Contributor

Landed in d179cca

Thank you, @vixwilli!

MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@MylesBorins
doc: add references to PR communication articles
3fcf073 
Added some references to PR communication articles in
Helpful Ressources inside COLLABORATOR_GUIDE.md

PR-URL: #17902
Fixes: #16359
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@MylesBorins
doc: add references to PR communication articles
d89cce5 
Added some references to PR communication articles in
Helpful Ressources inside COLLABORATOR_GUIDE.md

PR-URL: #17902
Fixes: #16359
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@MylesBorins
doc: add references to PR communication articles
2f7030d 
Added some references to PR communication articles in
Helpful Ressources inside COLLABORATOR_GUIDE.md

PR-URL: #17902
Fixes: #16359
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Jan 24, 2018
@MylesBorins
doc: add references to PR communication articles
68235da 
Added some references to PR communication articles in
Helpful Ressources inside COLLABORATOR_GUIDE.md

PR-URL: #17902
Fixes: #16359
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2018
@MylesBorins
doc: add references to PR communication articles
231c3bb 
Added some references to PR communication articles in
Helpful Ressources inside COLLABORATOR_GUIDE.md

PR-URL: #17902
Fixes: #16359
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
@MylesBorins
doc: add references to PR communication articles
aea3b74 
Added some references to PR communication articles in
Helpful Ressources inside COLLABORATOR_GUIDE.md

PR-URL: #17902
Fixes: #16359
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
@MylesBorins
doc: add references to PR communication articles
72b8924 
Added some references to PR communication articles in
Helpful Ressources inside COLLABORATOR_GUIDE.md

PR-URL: #17902
Fixes: #16359
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
@MylesBorins
doc: add references to PR communication articles
21c5d82 
Added some references to PR communication articles in
Helpful Ressources inside COLLABORATOR_GUIDE.md

PR-URL: #17902
Fixes: #16359
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@starkwang starkwang starkwang approved these changes

@gibfahn gibfahn gibfahn 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.
9 participants
@SalameWilliam @Trott @vsemozhetbyt @gibfahn @starkwang @jasnell @lpinca @MylesBorins @nodejs-github-bot