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: change STYLE-GUIDE to STYLE_GUIDE #11460

Closed
wants to merge 1 commit into from

Conversation

Dean-Coakley
Copy link
Contributor

@Dean-Coakley Dean-Coakley commented Feb 19, 2017

Changed filename of STYLE-GUIDE to STYLE_GUIDE to be consistent with naming conventions.
Changed all references to this file.

Fixes: #11456

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Feb 19, 2017
@Dean-Coakley Dean-Coakley changed the title Fixed style guide link Fixed style-guide link Feb 19, 2017
@Dean-Coakley Dean-Coakley changed the title Fixed style-guide link Fixed style-guide link in CONTRIBUTING.md Feb 19, 2017
@joyeecheung
Copy link
Member

The commit(ca5215e) that added this file actually says:

doc: add STYLE_GUIDE (moved from nodejs/docs)

So probably it's the file name that should be corrected? @gibfahn

@Dean-Coakley
Copy link
Contributor Author

Dean-Coakley commented Feb 19, 2017

@joyeecheung I think you are right. Pretty much every other file uses that naming convention. I am happy to close this.

Might wait for confirmation first.

Please excuse the side note: If anyone has any suggestions/ideas for my first contribution to Node elsewhere let me know! :) Documentation/Javascript only pretty much. Very minimal knowledge of C++.

@mscdex mscdex added the meta Issues and PRs related to the general management of the project. label Feb 19, 2017
@lpinca
Copy link
Member

lpinca commented Feb 19, 2017

@Dean-Coakley don't close, this PR could be updated to fix the file name instead of the link.

@gibfahn
Copy link
Member

gibfahn commented Feb 20, 2017

@Dean-Coakley yeah, I'd change the PR to update the file name (and fix any links that were pointing to the old file name).

cc/ @nodejs/docs

@gibfahn
Copy link
Member

gibfahn commented Feb 20, 2017

Please could you format your commit message according to the Contributing Guidelines?

Maybe something like this (if you include the Fixes: link the issue will automatically be closed when this PR lands):

doc: change STYLE-GUIDE to STYLE_GUIDE

Fixes: https://github.com/nodejs/node/issues/11456

If you don't have time it can be rewritten by whoever lands this, but if you are going to help out more on the project (as I hope you do), then it saves work if you produce better commit messages.

This seems like a good place to start with contributing, if you're looking for other stuff to do there is the good first contribution label.

@Dean-Coakley Dean-Coakley changed the title Fixed style-guide link in CONTRIBUTING.md doc: change STYLE-GUIDE to STYLE_GUIDE Feb 20, 2017
@Dean-Coakley
Copy link
Contributor Author

@gibfahn My apologise for these mistakes.

I think I have fixed everything but it appears to be two commits. Er...is this ok? Or must I rebase or something?

Thanks.

@lpinca
Copy link
Member

lpinca commented Feb 20, 2017

@Dean-Coakley commits can be squashed when landing by whoever lands this. If you want to help further you can squash the commits and add the Fixes: metadata in the commit message as @gibfahn suggested.

Thanks!

@claudiorodriguez
Copy link
Contributor

@Dean-Coakley I'll merge this tomorrow if no one objects - I'll rebase the commits and add metadata if you want, but if you want to do it yourself, you basically need to git rebase -i HEAD~2 on your local branch for this PR, mark the first commit as drop (we don't need to change the link), and the second as reword. You should get a prompt to edit the second commit's message, where you can then add the metadata (fixes, pr-url, reviewed-by). Then you save and after the rebase is done, you should only see the second commit with git log, and the link should be unaltered, and the file should be renamed. Then you push to your remote branch with --force and that should update the PR.

@gibfahn
Copy link
Member

gibfahn commented Feb 20, 2017

mark the first commit as drop (we don't need to change the link),

I don't think that will just work, the second commit reverts the change in the first one, so you'll probably get a merge conflict.

@claudiorodriguez
Copy link
Contributor

Oh, didn't notice that. Just squash then

@Dean-Coakley
Copy link
Contributor Author

So I squash both commits? Haven't done this before.

@claudiorodriguez
Copy link
Contributor

Mark the second commit as squash - since you always squash into a previous commit. Then you'll get a prompt to edit the commit message (with both commit messages pasted together - make sure only what you want remains)

@gibfahn
Copy link
Member

gibfahn commented Feb 20, 2017

@Dean-Coakley you should have something like this:

Rebasing and removing unneeded commits

image

Then when you save and quit, you'll get the commit message editor:

image

Delete the first commit message lines so it looks like:

image

Then save and quit. You should see some output like:

image

git status (which I have aliased to g s) should show that you're 2 commits behind, 1 commit ahead of your master branch. If you then do git push --force-with-lease it will overwrite/update your remote branch.

@Dean-Coakley
Copy link
Contributor Author

Dean-Coakley commented Feb 20, 2017

You are currently editing a commit while rebasing

No changes
You asked to amend the most recent commit, but doing so would make
it empty. You can repeat your command with --allow-empty, or you can
remove the commit entirely with "git reset HEAD^".

I should do git reset?

@gibfahn
Copy link
Member

gibfahn commented Feb 20, 2017

@Dean-Coakley are you on Twitter? It's difficult to have a conversation with someone on Github (I'm @gibfahn, feel free to ping me there).

EDIT: or IRC?

@gibfahn
Copy link
Member

gibfahn commented Feb 20, 2017

@Dean-Coakley Looks like something went wrong (you've now got 4 commits in this PR). Here's what I'd suggest you do (this assumes you followed CONTRIBUTING.md and now have two remotes, origin and upstream.

git reset --hard upstream/master # Clean everything and start again
mv doc/STYLE-GUIDE.md doc/STYLE_GUIDE.md # Make the change again
git add -A # Add all files to the index
git commit
# Paste in the commit message, I've put it in below
git status # Should say 5 commits behind, 1 commit ahead
git push --force-with-lease

Try that

doc: change STYLE-GUIDE to STYLE_GUIDE

Fixes: https://github.com/nodejs/node/issues/11456

@Dean-Coakley
Copy link
Contributor Author

Should be all good now.

Thanks everyone for your help! 😄

@gibfahn
Copy link
Member

gibfahn commented Feb 21, 2017

Will land later today if there are no objections.

@gibfahn gibfahn self-assigned this Feb 21, 2017
@Dean-Coakley Dean-Coakley force-pushed the master branch 3 times, most recently from 0349d70 to ed1a497 Compare February 21, 2017 21:49
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

gibfahn pushed a commit that referenced this pull request Feb 22, 2017
PR-URL: #11460
Fixes: #11456
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@gibfahn
Copy link
Member

gibfahn commented Feb 22, 2017

Landed in 5f08871

Thanks for your contribution @Dean-Coakley!

@gibfahn gibfahn closed this Feb 22, 2017
addaleax pushed a commit that referenced this pull request Feb 22, 2017
PR-URL: #11460
Fixes: #11456
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@italoacasas italoacasas mentioned this pull request Feb 25, 2017
@jasnell
Copy link
Member

jasnell commented Mar 7, 2017

This should only land in v6 or v4 if the original style guide PR lands

@gibfahn
Copy link
Member

gibfahn commented Mar 7, 2017

(Depends on #11321)

jasnell pushed a commit that referenced this pull request Mar 7, 2017
PR-URL: #11460
Fixes: #11456
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
PR-URL: #11460
Fixes: #11456
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
PR-URL: #11460
Fixes: #11456
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
PR-URL: #11460
Fixes: #11456
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
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. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants