-
Notifications
You must be signed in to change notification settings - Fork 520
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
feat: recommend imperative mood, with emphais for fix
#196
base: master
Are you sure you want to change the base?
feat: recommend imperative mood, with emphais for fix
#196
Conversation
fix
fix
@bcoe you're a native speaker, so you can review this one better than me :) |
@Artoria2e5 bother you to update your PR to reflect the |
content/next/index.md
Outdated
@@ -96,7 +96,8 @@ The key words “MUST”, “MUST NOT”, “REQUIRED”, “SHALL”, “SHALL | |||
1. A scope MAY be provided after a type. A scope MUST consist of a noun describing a | |||
section of the codebase surrounded by parenthesis, e.g., `fix(parser):` | |||
1. A description MUST immediately follow the colon and space after the type/scope prefix. | |||
The description is a short summary of the code changes, e.g., _fix: array parsing issue when multiple spaces were contained in string._ | |||
The description is a short summary of the code changes, e.g., _fix: don't ignore consecutive spaces when parsing array._ | |||
1. When the type field is a verb, the description MAY be an object or action the verb is applied to, only when the change is trivial or clear enough from the type, e.g., _revert: drop Node 6 from testing matrix._ Nontrivial changes, especially those of a `fix` type, SHOULD describe the changeset itself rather than the undesirable behavior it removes. |
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.
I think I'm more comfortable with an FAQ suggestion, that speaks to some of these recommendations. It seems like we could keep the formal spec text centered mainly on logic that could be adopted by a parser; whereas this recommendation is more a guideline for how one might go about writing commit messages ...
It almost makes me wonder if we'd do better to add a "related reading" section, rather than continuing to expand the FAQ.
Add imperative present tense as our recommended writing form to the FAQ. Closes conventional-commits#84 conventional-commits#77
You can use it as the verb for your description only if it's trivial enough.
7e2fb16
to
3496a0b
Compare
|
||
We recommend writing a commit description and body using the [imperative](https://en.wikipedia.org/wiki/Imperative_mood) present tense writing form. There are a significant number of examples of this writing form for commits: [1](https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html)[2](https://github.com/angular/angular/blob/22b96b9/CONTRIBUTING.md#subject)[3](https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project)[4](https://medium.com/@danielfeelfine/commit-verbs-101-why-i-like-to-use-this-and-why-you-should-also-like-it-d3ed2689ef70)[5](https://chris.beams.io/posts/git-commit/#imperative) | ||
|
||
If you are using a verb, such as "update" and "revert" as a type, you may see it as the verb of the imperative phrase only if the change is really trivial. For more complex things such as "fix", you should stick to describing what exactly you have done using the imperative form. Writing things like _fix: crash on login_ is a huge no-no. |
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.
Do not really agree with it fix: crash on login
. Because it may have a description to explain more details.
Could you please elaborate more?
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.
I can rephrase it to "Writing things like... without further description in the commit body is a huge no-no. You should at least try to mention the root cause."
The story is that I got fed up with someone else writing fix: fooBarProcess jams up console
when fix: turn off console logging for fooBarProcess
works much better. Crashes and more complex bugs are indeed harder to describe in 44 characters, although people should still try to write stuff like fix: free up fooObj to avoid instant OOM death
in my opinion. The "huge" part really needs some re-considering.
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.
Any update on this @Artoria2e5 ?
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.
The imperative style is quite well established good practice. See for example https://www.git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project
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.
I do aggree with imperative style, but I don't aggree with this specific example.
The commit-message, especially in the context of conventional-commit, should be a user-oriented changelog. And a user shouldn't have to care nor understand the actual cause of the issue. We only tell the user what is fixed, not the internal details that caused the bug. Ofc, the message should describe under what conditions the bug was appearing, exaample: fix: OOM when calling fooObj
The body of the commit message may eventually contains some more in-depth/developer-oriented explanation of what was the root cause how it is fixed.
Any update @Artoria2e5 ? |
I commented over in #190 and didn't realize there was a PR for this: #190 (comment)
|
@Artoria2e5 any update? I'd love to see this one landing tho |
@Artoria2e5 is apparently "Functionally asleep", despite this issue remaining as relevant. How can this merge request be appropriately merged nonetheless? |
This closes #190. The first commit is recovered from #85, which got reverted somehow some time later for some reason.