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: reduce ambiguity around footer/body #185

Merged
merged 7 commits into from
Sep 7, 2019
Merged

feat: reduce ambiguity around footer/body #185

merged 7 commits into from
Sep 7, 2019

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Aug 29, 2019

Working from @JeanMertz's suggestions in #179 proposes a few new rules related to differentiating between footer and body sections of commit message.

Builds on top of git trailers and RFC822 folding conventions.

(I also did some minor editing, and removed some text I felt was redundant, given that the spec is growing a little bit).

fixes: #170

CC: @epage, @JeanMertz, @blowmage

content/next/index.md Outdated Show resolved Hide resolved
Copy link
Member

@zeke zeke left a comment

Choose a reason for hiding this comment

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

👍

Co-Authored-By: Zeke Sikelianos <zeke@sikelianos.com>
@bcoe bcoe mentioned this pull request Aug 29, 2019
Copy link

@blowmage blowmage left a comment

Choose a reason for hiding this comment

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

👍

Copy link

@JeanMertz JeanMertz left a comment

Choose a reason for hiding this comment

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

Great, thank you for writing this @bcoe. I didn't get around to it last weekend, so I'm glad you did.

I added two remarks that I think deserve some discussion/rethinking, but other than that, this looks great 👍.

content/next/index.md Outdated Show resolved Hide resolved
content/next/index.md Outdated Show resolved Hide resolved
@damianopetrungaro
Copy link
Member

@bcoe apart from the comment I left I do really like the work!

bcoe and others added 2 commits September 6, 2019 21:06
Co-Authored-By: Damiano Petrungaro <damianopetrungaro@gmail.com>
Copy link
Member

@tunnckoCore tunnckoCore left a comment

Choose a reason for hiding this comment

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

couple of comments :)

@@ -21,7 +21,7 @@ The commit message should be structured as follows:

[optional body]

[optional footer]
[optional footer(s)]
Copy link
Member

Choose a reason for hiding this comment

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

not agree. technically, there's one footer with probably multiple sections. but anyway.

A BREAKING CHANGE can be part of commits of any _type_.
1. Others: commit _types_ other than `fix:` and `feat:` are allowed, for example [@commitlint/config-conventional](https://github.com/conventional-changelog/commitlint/tree/master/%40commitlint/config-conventional) (based on the [the Angular convention](https://github.com/angular/angular/blob/22b96b9/CONTRIBUTING.md#-commit-message-guidelines)) recommends `chore:`, `docs:`, `style:`, `refactor:`, `perf:`, `test:`, and others.
1. _types_ other than `fix:` and `feat:` are allowed, for example [@commitlint/config-conventional](https://github.com/conventional-changelog/commitlint/tree/master/%40commitlint/config-conventional) (based on the [the Angular convention](https://github.com/angular/angular/blob/22b96b9/CONTRIBUTING.md#-commit-message-guidelines)) recommends `chore:`, `docs:`, `style:`, `refactor:`, `perf:`, `test:`, and others.
1. _footers_ other than `BREAKING CHANGE: <description>` may be be provided and and follow a convention similar to
Copy link
Member

Choose a reason for hiding this comment

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

double "be" and double "and". ;)

1. Breaking changes MUST be indicated in the type/scope prefix of a commit, or at the beginning of its optional body or footer section.
1. If included in the body or footer, a breaking change MUST consist of the uppercase text BREAKING CHANGE, followed by a colon, space, and description, e.g.,
1. A commit body is free-form and MAY consist of any number of newline separated paragraphs.
1. One or more footers MAY be provided one blank line after the body. Each footer MUST consist of
Copy link
Member

Choose a reason for hiding this comment

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

I don't think and don't like to promote that as multiple footers. Better, in my mind, is to promote it as footer containing multiple sections (git trailers)

1. The units of information that make up conventional commits MUST NOT be treated as case sensitive by implementors, with the exception of BREAKING CHANGE which MUST be uppercase.
1. BREAKING-CHANGE MUST be synonymous with BREAKING CHANGE, when used as a token in a footer.
Copy link
Member

Choose a reason for hiding this comment

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

hm, so the trailer is BREAKING-CHANGE: <description>? And without the hyphen won't be valid anymore? Maybe adding and example as next sentence would be great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I think we should support both approaches, since it's confusing to not support breaking-change, since it's closer to other footers.

Copy link
Member

Choose a reason for hiding this comment

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

Yea. So the BREAKING CHANGE: will still be supported too?

@skeggse
Copy link

skeggse commented Nov 1, 2019

I'm noticing that this change resulted in the removal of the improvement commit type from v1.0.0 of the spec. I realize that this doesn't mean it's forbidden, but I do wonder whether this should have any bearing on tools that implemented the improvement commit type - given that at least one added it due to its explicit inclusion in the spec.

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.

Confusion over "body"
7 participants