Skip to content

Conversation

@tanujasawant
Copy link
Contributor

@tanujasawant tanujasawant commented Nov 1, 2016

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

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 1, 2016
@Fishrock123
Copy link
Contributor

R=@Trott

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@thefourtheye
Copy link
Contributor

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

@tanujasawant tanujasawant changed the title doc: update COLLABORATOR_GUIDE and onboarding to mention core-validat… doc: update COLLABORATOR_GUIDE and onboarding Nov 1, 2016
@tanujasawant
Copy link
Contributor Author

@thefourtheye, would you kindly check now?
Thanks!

@tanujasawant
Copy link
Contributor Author

#1003 in nodejs/nodejs.org

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: Remove Additionally:

Copy link
Contributor Author

@tanujasawant tanujasawant Nov 2, 2016

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

@Trott Please check the latest commit
Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: validates -> can be used to validate

@Trott
Copy link
Member

Trott commented Nov 2, 2016

@Trott Please check the latest commit

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

@tanujasawant
Copy link
Contributor Author

tanujasawant commented Nov 3, 2016

@Trott Please check

Copy link
Member

Choose a reason for hiding this comment

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

Nit: Remove whitespace on empty line.

@Trott
Copy link
Member

Trott commented Nov 3, 2016

Works for me. Thanks!

@tanujasawant
Copy link
Contributor Author

tanujasawant commented Nov 4, 2016

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
Copy link
Member

gibfahn commented Nov 4, 2016

@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.

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.

7 participants