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

feat: recommend imperative mood, with emphais for fix #196

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Artoria2e5
Copy link
Contributor

@Artoria2e5 Artoria2e5 commented Sep 11, 2019

This closes #190. The first commit is recovered from #85, which got reverted somehow some time later for some reason.

@Artoria2e5 Artoria2e5 changed the title feat: recommend imperative mood for fix feat: recommend imperative mood, with emphais for fix Sep 11, 2019
@damianopetrungaro
Copy link
Member

@bcoe you're a native speaker, so you can review this one better than me :)

@bcoe
Copy link
Contributor

bcoe commented Sep 30, 2019

@Artoria2e5 bother you to update your PR to reflect the 1.0.0 version of the specification we just landed?

@@ -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.
Copy link
Contributor

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.

hutson and others added 2 commits September 30, 2019 15:26
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.

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

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?

Copy link
Contributor Author

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.

Copy link
Member

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 ?

Copy link
Contributor

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

Copy link

@jcornaz jcornaz Dec 23, 2021

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.

@damianopetrungaro
Copy link
Member

Any update @Artoria2e5 ?
Do you still have time to deliver this? So much value! We'd love to see you as contributor for this!

@wesleytodd
Copy link

I commented over in #190 and didn't realize there was a PR for this: #190 (comment)

I am not sure that the specification can add much value in this area. I think that a guide/faq could provide recommendations, but as there is now way to validate/parse or otherwise tool around the meaning of the description I don't think there is value in the spec trying to detail expectations there.

@damianopetrungaro
Copy link
Member

@Artoria2e5 any update? I'd love to see this one landing tho

@TristanCottam
Copy link

@Artoria2e5 is apparently "Functionally asleep", despite this issue remaining as relevant. How can this merge request be appropriately merged nonetheless?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ambiguity around description field of "fix"
8 participants