-
Notifications
You must be signed in to change notification settings - Fork 39
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: add examples in commit message guidelines #131
Conversation
ef0ac47
to
97b60c5
Compare
seeing if old issues can be closed with recent changes | ||
|
||
|
||
Message examples |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specific examples are up to debate, and I think having more would be beneficial. For now I added a simple example, in future this will be expanded
The `git`:cmd: stuff | ||
==================== | ||
|
||
PR should contain a single commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More elaborate explanation about reasons and solutions to the "single commit" rule, also need to be checked for sanity.
@@ -45,6 +45,17 @@ Code reviews | |||
When reviewing code it is highly recommended to check whether it complies | |||
with official style guide and point out mismatches. | |||
|
|||
Prepared replies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More replies need to be added here, for now I only added one that links to the commit message guidelines
edc7f15
to
f5d8112
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. 🥳
Saw a few things you might want to change, please see my comments and suggestions. Thanks for jumping on this right quick.
|
||
.. code:: | ||
|
||
Fixes https://github.com/nim-lang/Nim/issues/19051 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes https://github.com/nim-lang/Nim/issues/19051 | |
Fixes https://github.com/nim-works/nimskull/issues/19051 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That one should retain the original url, we don't have a 19k issues at the moment
- If you need to made some additional modifications (review requested) you | ||
can extend the commit and force-push it (`git commit --amend | ||
--no-edit`:cmd: and `git push --force <remote> <your branch>`:cmd:) | ||
- Create multiple commits and then squash them when your pull request is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we elevate this one, it's a nicer flow and less frustrating. It's one of those things where people don't realize they can do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is more frustrating to do right now, requires digging into multistep git stuff create an intermediate branch. Extending the same commit and force-pushing it is much easier, especially for a small changes, where you just need to correct a couple of lines.
Add more elaborate examples in the commit message guidelines and pull requests. Provide reasoning and reformat rules
f5d8112
to
d49d943
Compare
bors r+ |
Build succeeded: |
Add more elaborate examples in the commit message guidelines and pull
requests. Provide reasoning and reformat rules