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.

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: split CONTRIBUTING.md #18271

Closed
wants to merge 5 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jan 20, 2018

I wanted to update the CONTRIBUTING.md with some notes on CI flakes, but seeing how big it is I decided to split it first. It is already too long to read.

The first commit split the CONTRIBUTING.md into separate documents. No changes are made to the text of each section other than relative link fixes.
The second commit improves the summaries in CONTRIBUTING.md.

Fixes: #17842

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jan 20, 2018
@joyeecheung
Copy link
Member Author

Added another commit to fix the external references to CONTRIBUTING.md

cc @Trott @nodejs/documentation

@targos
Copy link
Member

targos commented Jan 20, 2018

Thanks for doing that!

I have the feeling that additional notes could be in pull-requests.md with two changes:
Remove "Helpful Resources" and:

  • move the link to core-validate-commit to the end of the "Commit message guidelines" section.
  • move the MCVE link to "Submitting a Bug Report" in issues.md.

@joyeecheung
Copy link
Member Author

@targos Thanks for the review. I have moved additional notes into pull-requests.md. As for the links, I would like to update them in a later PR. The pull-requests.md is still too big and we should probably do a further clean up later. What do you think?

@targos
Copy link
Member

targos commented Jan 20, 2018

SGTM. Thanks!

@joyeecheung
Copy link
Member Author

@targos Ah, after reading it again I think you are right, they look pretty out-of-the-place like this, we may as well update them now. I have updated the links per your suggestions. Thanks.

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 22, 2018
@joyeecheung
Copy link
Member Author

@joyeecheung
Copy link
Member Author

Landed in 695ed67, thanks!

joyeecheung added a commit that referenced this pull request Jan 23, 2018
PR-URL: #18271
Fixes: #17842
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@joyeecheung joyeecheung removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 23, 2018
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
PR-URL: #18271
Fixes: #17842
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

Lands on 8.x but a couple conflict on 6.x. We likely need to backport a bunch of different doc fixes that haven't landed cleanly on 6.x to get this to work so I'm setting it as don't land

MylesBorins pushed a commit that referenced this pull request Feb 27, 2018
PR-URL: #18271
Fixes: #17842
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@benjamingr
Copy link
Member

@MylesBorins is there any reason why files like CONTRIBUTING.md (and the readme, and COLLABORATOR_GUIDE etc) need to be backported at all?

@trivikr trivikr mentioned this pull request Mar 20, 2018
2 tasks
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#18271
Fixes: nodejs#17842
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CONTRIBUTING.md improvements
8 participants