doc: update COLLABORATOR_GUIDE and onboarding #9396

tanujasawant · 2016-11-01T11:34:04Z

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

Description of change

COLLABORATOR_GUIDE.md and doc/onboarding.md are updated to mention
core-validate-commit as a useful tool in validating commit messages.
Ref: #9328

Fishrock123 · 2016-11-01T13:43:48Z

R=@Trott

thefourtheye · 2016-11-01T13:46:21Z

COLLABORATOR_GUIDE.md

@@ -125,6 +125,7 @@ Additionally:
 - Except when updating dependencies, all commits should be self
  contained (meaning every commit should pass all tests). This makes
  it much easier when bisecting to find a breaking change.
+- The [core-validate-commit](https://github.com/evanlucas/core-validate-commit#core-validate-commit) command validates the commit message for a particular commit in node core.


Can you break this line in to two lines and align the second line right below the first one?

Also, the hashtag might not be absolutely necessary, as the README is right there in the landing page.

thefourtheye · 2016-11-01T13:50:55Z

Can you please format the commit message as per these guidelines?

tanujasawant · 2016-11-01T16:09:30Z

@thefourtheye, would you kindly check now?
Thanks!

tanujasawant · 2016-11-01T16:37:57Z

#1003 in nodejs/nodejs.org

Trott

This change mentions the tool, but is missing useful context.

The onboarding doc mentions it as a step after git push upstream master which is likely to give people the wrong idea about when they should run the command.
Is the tool required or suggested? (It's suggested.)
How/when do you run it? (Provide an example command and sample output.)
I think this should be in onboarding-extras.md and linked to from onboarding.md.

Trott · 2016-11-01T21:05:31Z

doc/onboarding.md

@@ -204,6 +204,7 @@ Landing a PR
 * Optional: Force push the amended commit to the branch you used to open the pull request. If your branch is called `bugfix`, then the command would be `git push --force-with-lease origin master:bugfix`. When the pull request is closed, this will cause the pull request to show the purple merged status rather than the red closed status that is usually used for pull requests that weren't merged. Only do this when landing your own contributions.
 * `git push upstream master`
    * Close the pull request with a "Landed in `<commit hash>`" comment.
+* Additionally: The [core-validate-commit](https://github.com/evanlucas/core-validate-commit) command validates the commit message for a particular commit in node core.


Nit: Remove Additionally:

@Trott, would it be appropriate of onboarding.md to point to merely two lines in onboarding-extras.md? Or shall I include it in onboarding.md itself?

Shall I write Optional: or Suggested: before the bullet point to point to the fact that the core-validate-commit is not a required tool?

tanujasawant · 2016-11-02T08:38:52Z

@Trott Please check the latest commit
Thanks!

Trott · 2016-11-02T22:20:50Z

doc/onboarding.md

@@ -204,6 +204,7 @@ Landing a PR
 * Optional: Force push the amended commit to the branch you used to open the pull request. If your branch is called `bugfix`, then the command would be `git push --force-with-lease origin master:bugfix`. When the pull request is closed, this will cause the pull request to show the purple merged status rather than the red closed status that is usually used for pull requests that weren't merged. Only do this when landing your own contributions.
 * `git push upstream master`
    * Close the pull request with a "Landed in `<commit hash>`" comment.
+* Optional: The [core-validate-commit](https://github.com/evanlucas/core-validate-commit) is a useful tool to help validate the commit message for a particular commit in node core. Refer [this](https://github.com/evanlucas/core-validate-commit/blob/master/bin/usage.txt) for examples.


I know I said to include usage and examples, but the link is superfluous. Just remove the second sentence entirely. The first sentence is fine, but the entire bullet point should be moved above the line 204, as you'd want to validate the commit after applying metadata but before pushing the commit to a repository.

Trott · 2016-11-02T22:21:10Z

COLLABORATOR_GUIDE.md

@@ -125,7 +125,9 @@ Additionally:
 - Except when updating dependencies, all commits should be self
  contained (meaning every commit should pass all tests). This makes
  it much easier when bisecting to find a breaking change.
-
+- The [core-validate-commit](https://github.com/evanlucas/core-validate-commit)
+  command validates the commit message for a particular commit in node core.


Nit: validates -> can be used to validate

Trott · 2016-11-02T22:22:11Z

@Trott Please check the latest commit

Getting closer. Still a few small issues (which I've left comments about).

tanujasawant · 2016-11-03T02:22:33Z

@Trott Please check

Trott · 2016-11-03T05:40:36Z

COLLABORATOR_GUIDE.md

+- The [core-validate-commit](https://github.com/evanlucas/core-validate-commit)
+  command can be used to validate the commit message for a particular 
+  commit in node core.
+


Nit: Remove whitespace on empty line.

Trott · 2016-11-03T05:41:47Z

Works for me. Thanks!

tanujasawant · 2016-11-04T15:16:44Z

I made changes to the associated branch by mistake. Consequently, this PR shows to be closed. This wouldn't affect your ability to merge the PR, would it?
@jasnell @Trott The commits are not showing!

gibfahn · 2016-11-04T17:50:52Z

@Tanuja-Sawant Github doesn't keep save the commits you added for this PR, it just compares nodejs:master and your PR branch (in this case Tanuja-Sawant:master). If you change what your branch points to then we can't find the commits.

You should be able to get your commits back though. If you type git reflog master it should show you a history of the commits master has pointed to. Those commits are kept around until git cleans them up, which it does only occasionally. If you find the last good commit, you can reset your branch to it with git checkout master, git reset --hard afeccd (or whatever the hash is). See this guide for more info. Once you fix your branch you should be able to reopen this PR.

Edit: If you have other changes that you want to keep on your master branch, you should create a new branch for them before you do the git reset. You can do git checkout master, git branch my-new-branch to create a new branch for the new changes, then do the reflog and reset commands.

That's why it's generally a good reason to create a named branch for each pr (I normally do something like pr-validate-commit) so you don't accidentally overwrite your branch. CONTRIBUTING.md has all the info on that.

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 1, 2016

thefourtheye reviewed Nov 1, 2016

View reviewed changes

tanujasawant changed the title ~~doc: update COLLABORATOR_GUIDE and onboarding to mention core-validat…~~ doc: update COLLABORATOR_GUIDE and onboarding Nov 1, 2016

tanujasawant mentioned this pull request Nov 1, 2016

locale: update development.md nodejs/nodejs.org#1003

Merged

Trott requested changes Nov 1, 2016

View reviewed changes

Trott reviewed Nov 2, 2016

View reviewed changes

Trott reviewed Nov 3, 2016

View reviewed changes

Trott approved these changes Nov 3, 2016

View reviewed changes

jasnell approved these changes Nov 3, 2016

View reviewed changes

tanujasawant closed this Nov 4, 2016

Uh oh!

doc: update COLLABORATOR_GUIDE and onboarding #9396

doc: update COLLABORATOR_GUIDE and onboarding #9396

Uh oh!

Conversation

tanujasawant commented Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Fishrock123 commented Nov 1, 2016

Uh oh!

thefourtheye Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

thefourtheye Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented Nov 1, 2016

Uh oh!

tanujasawant commented Nov 1, 2016

Uh oh!

tanujasawant commented Nov 1, 2016

Uh oh!

Trott left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

tanujasawant Nov 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanujasawant Nov 2, 2016

Choose a reason for hiding this comment

Uh oh!

tanujasawant commented Nov 2, 2016

Uh oh!

Trott Nov 2, 2016

Choose a reason for hiding this comment

Uh oh!

Trott Nov 2, 2016

Choose a reason for hiding this comment

Uh oh!

Trott commented Nov 2, 2016

Uh oh!

tanujasawant commented Nov 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott Nov 3, 2016

Choose a reason for hiding this comment

Uh oh!

Trott commented Nov 3, 2016

Uh oh!

tanujasawant commented Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gibfahn commented Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

tanujasawant commented Nov 1, 2016 •

edited

Loading

Trott left a comment •

edited

Loading

tanujasawant Nov 2, 2016 •

edited

Loading

tanujasawant commented Nov 3, 2016 •

edited

Loading

tanujasawant commented Nov 4, 2016 •

edited

Loading

gibfahn commented Nov 4, 2016 •

edited

Loading