Resolving ambiguity in the spec

[mrwilson]

Hi!

I'm a big fan of conventional commits, and have been using it on most of our work projects and all of my personal projects, so thank you.

I've been trying to implement a parser for the spec here and have a question.

Given

A longer commit body MAY be provided after the short description, providing additional contextual information about the code changes. The body MUST begin one blank line after the description.

and

A footer MAY be provided one blank line after the body (or after the description if body is missing).

How can automated process tell the difference between a commit message with just a description and a commit message with just a footer?

Thanks,

Alex

[damianopetrungaro]

Hey @mrwilson thank you for the issue!

At the moment there's no way to distinguish them. Do you have an issue with it? If you have an idea to improve the specs you're welcome!

[mrwilson]

Hi,

My issue is that it means I can't properly implement a parsing library for the spec (yet!), and I thought it would be good to have a reference implementation for the spec?

How would I go about proposing an amendment to the spec? I have an idea.

Thanks! :)

[damianopetrungaro]

Feel free to suggest it here! Any idea is welcome! 💪

[mrwilson]

Thanks! Here is a rough outline and I'm happy to formalise it into proper language if acceptable.

Proposal

Amend the spec to require delimiting 'body' with bullets (whether * or -) and leave 'footer' as a bare line.

Examples

We've informally adopted this at my workplace and we've found it an easy step - so you end up with commits like below, using https://github.com/unruly/hiera-secrets-manager/commit/e9b5f40e16d1782636521fee99923648ae8ad846 as an example:

feat(hash-support): Support Hashes from Secrets Manager

* This is to simplifies the number and namespace of the secrets stored in secrets manager
* It allows us to update a set of interdependent creds like AWS keypairs at the same time - avoids race conditions causing invalid credentials combinations

Co-authored-by: Narayan O'Hanlon <narayanohanlon@gmail.com>
Co-authored-by: Alex Wilson <a.wilson@alumni.warwick.ac.uk>

So 'body' is the section delimited by * and the footer contains the Co-Authored-By statements.

I believe this also increases readability by empowering spec-followers to clearly separate different points.

Interoperability with Existing Spec

This doesn't interfere with existing semantics around e.g. BREAKING CHANGE, as in the body these would be subsumed into the line as below:

* BREAKING CHANGE: This is a breaking change

BREAKING CHANGE: This is also valid as a breaking change.

How does this sound as a sketch?

[damianopetrungaro]

I don't think this implementation may solve the issue in a fashionable way. You're adding noise to the commit message and it would be harder to read and understand.

See that you already have space as a separator for type+description/body/footer it doesn't make sense really to add a Co-authored-by that may not fit all the use cases (for example link a Jira issue).

IMHO it may be sth way simpler such as a ___. An example:

<type>[optional scope]: <description>

[optional body]

[optional footer separator ___]
[optional footer]

And you can omit it if you do not have a footer.

Before continuing the discussion i would like to ask an opinion from other maintainers @zeke @bcoe @hbetts

[mrwilson]

I see what you mean - by the way, the Co-Authored-By could be anything, it's just that's the kind of thing we put into our footers to attribute commits. The only change I'm proposing is bulletting the body section. For clarification, a generic example would be:

fix(widgets): Widgets were rusty

* We shouldn't expose them to wind and rain.

BREAKING CHANGE: Gear ratio is different for new widgets.

I'd be happy with the separator solution too :)

[hutson]

At the moment there's no way to distinguish them. - @damianopetrungaro

If there is no way to distinguish between them, then why have specify two separate sections?

The only change I'm proposing is bulletting the body section. - @mrwilson

Any change to the spec that requires additional effort on the part of a committer would need to provide compelling value. Every additional requirement increases the likelihood of it not being followed correctly.

---

My opinion is that the specification does not provide justification for having two separate sections, a footer and a body.

[damianopetrungaro]

The point is that IMHO there's no value splitting them.

The body+footer section can always be used together and could be treated as a better explanation of the commit.

[bcoe]

one option, perhaps you can't have a footer without a body? and if there are just two sections, always assume header and body?

[damianopetrungaro]

@bcoe Both are optional so you may have only the body, only the footer or both of them.

[bcoe]

why don't we indicate that the footer is an optional part of the body but can't exist without the body; wipes hands, done.

I think this might fit with what the angular folks are also advocating.

[mrwilson]

As a day-to-day user, I personally prefer 'having only one section' to 'footer only exists within body'.

For both work and personal projects, I mainly use the footer for attributing commit authors but it's common to have a footer but not a body. (see https://github.com/unruly/hiera-secrets-manager/commit/1c776e063d8a705e4e2a82e2f8fff264a5e06df3 for an example)

[damianopetrungaro]

Maybe we can find a name for the entire section that includes body and footer so that there is no BC in the tools that are supporting conventional commit. wdyt @bcoe ?

[bcoe]

@stevemao @ajoslin @Tapppi @hbetts any thoughts about how to fix this in the spec? my concern is that the conventional-commits parsers do parse the footer I believe? how does the parser deal with this ambiguity?

[mrwilson]

I think it'd be good to add a reference implementation too, whichever way this gets resolved. That can be a separate issue though.

[zeke]

how does the parser deal with this ambiguity?

Here's what parse-commit-message does:

const elements = commitMessage.split(/\r?\n\r?\n/);
const headerStr: string = elements[0];
const body: string | null = elements.length > 2 ? elements[1] : null;
const footer: string = elements.length > 2 ? elements[2] : elements[1];

[bcoe]

@zeke et al., this seems reasonable to me; if there are two more elements, it's a body and footer, if there's one more element, it's always assumed to just be a body.

Personally I don't think we need to overcomplicate things for the edge-case of a footer, let's just make sure the language of the spec reflects the conditional nature of the footer.

[zeke]

if there's one more element, it's always assumed to just be a body.

That makes sense to me, but I think the implementation above is actually different: footer seems to take precedence over body. Maybe @tunnckoCore had a reason for writing it this way?

[damianopetrungaro]

@zeke

Maybe we can find a name for the entire section that includes body and footer so that there is no BC in the tools that are supporting conventional commit.

[bcoe]

@zeke @@tunnckoCore whoops, misread; this feels weird; my workflow, which I've found works quite well, is that I almost always only write the type and description line, unless it's a BREAKING CHANGE, at which point I fill in a body with a thorough description of the breaking change...

I can imagine a workflow where I use a footer, e.g., Node.js populating this with reviewers, but it definitely feels like the less frequently used section of a commit message.

tldr; my preference would be if there are three sections, assume the third is footer if there are two assume the second is body.

[bcoe]

☝️ I think we could describe this with a tiny bit of clarification in the section of the spec around footers, without adding much to the spec?

[mrwilson]

@bcoe - it sounds like it would be enough to replace

7. A footer MAY be provided one blank line after the body (or after the description if body is missing). The footer SHOULD contain additional issue references about the code changes (such as the issues it fixes, e.g.,Fixes #13).

with

7. A footer MAY be provided one blank line after the body. The footer SHOULD contain additional issue references about the code changes (such as the issues it fixes, e.g.,Fixes #13).

Happy to open a pull-request.

[tunnckoCore]

Glance reading through the thread, i'm agree. I had no some special reason to did it that way.

[bcoe]

@mrwilson 👍 sounds great to me, could I convince you to make a pull request 😄 many hands make for light lifting.

[mrwilson]

@bcoe I have done so, hope I made the change in the right place!

[damianopetrungaro]

Yes, next is the right place 😄

[hutson]

conventional-commits-parser is kind of a reference implementation though it's regular expressions are pretty accepting of most commit message conventions.

[mrwilson]

@hbetts Cool - I discovered the spec issue by trying to implement a module that encompasses the spec using a parser library.

Here if you're interested: https://github.com/mrwilson/conventional-commit/