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

Breaking change description #535

Open
epage opened this issue Aug 7, 2023 · 7 comments
Open

Breaking change description #535

epage opened this issue Aug 7, 2023 · 7 comments

Comments

@epage
Copy link

epage commented Aug 7, 2023

If included in the type/scope prefix, breaking changes MUST be indicated by a ! immediately before the :. If ! is used, BREAKING CHANGE: MAY be omitted from the footer section, and the commit description SHALL be used to describe the breaking change.

A straight reading of this makes it sound like that if ! is used, the commit description describes the breaking change, regardless of whether BREAKING CHANGE is present. This seems odd to me that we'd be throwing out the more specific BREAKING CHANGE description in favor of a description that may not be relevant (since ! is also meant to help draw attention to a breaking change).

Is that straight reading correct or was the following intended

If ! is used, BREAKING CHANGE: MAY be omitted from the footer section and if it is omitted then the commit description SHALL be used to describe the breaking change.

@ttd2089
Copy link

ttd2089 commented Aug 7, 2023

If included in the type/scope prefix, breaking changes MUST be indicated by a ! immediately before the :. If ! is used, BREAKING CHANGE: MAY be omitted from the footer section, and the commit description SHALL be used to describe the breaking change.

A straight reading of this makes it sound like that if ! is used, the commit description describes the breaking change, regardless of whether BREAKING CHANGE is present. This seems odd to me that we'd be throwing out the more specific BREAKING CHANGE description in favor of a description that may not be relevant (since ! is also meant to help draw attention to a breaking change).

Is that straight reading correct or was the following intended

If ! is used, BREAKING CHANGE: MAY be omitted from the footer section and if it is omitted then the commit description SHALL be used to describe the breaking change.

I suppose this also raises a question of whether a commit is allowed to express multiple breaking changes and, if so, how the list would be extracted from the commit.

If a commit is not allowed to express multiple breaking changes

In this case I think @epage has the right idea. The BREAKING CHANGE footer describes the breakage which may or may not be the best description for the change as a whole; e.g.

feat: enable custom formatting for foo

BREAKING CHANGE: The previously-default formatting for foo must now be explicitly specified.

It might be worth explicitly stating the semantics of the !; does it mean that the description describes the breaking change, or simply that the commit contains a breaking change? I had assumed the former given the clause "the commit description SHALL be used to describe the breaking change" in section 13; but @epage makes a good point that BREAKING CHANGE being thrown out seems odd. If it's the former then perhaps having both ! and BREAKING CHANGE should be disallowed for clarity's sake? In either case it seems like multiple BREAKING CHANGE footers should be disallowed since only one could be meaningful.

If a commit is allowed to express multiple breaking changes

In this case the same question about the semantics of the ! arises. I suppose that if it means the description describes the breaking change, then the description and all BREAKING CHANGES footers would each describe breaking changes. If it just means that the commit contains a breaking change then having no BREAKING CHANGE footers would mean the description describes the breaking change, and having one or more BREAKING CHANGE footers would mean that they each describe a breaking change but that the description does not.

edit: grammar

@epage
Copy link
Author

epage commented Aug 7, 2023

The BREAKING CHANGE footer describes the breakage which may or may not be the best description for the change as a whole; e.g.

On a personal, practical note, I frequently run into this case.

@ttd2089
Copy link

ttd2089 commented Aug 7, 2023

The BREAKING CHANGE footer describes the breakage which may or may not be the best description for the change as a whole; e.g.

On a personal, practical note, I frequently run into this case.

Yea I'm kind of torn now... I 100% agree that the scenario you describe has to be supported but I think the ! VS BREAKING CHANGE thing comes down to whether the following commits mean the same thing or not:

feat: foo

BREAKING CHANGE: bar
feat!: foo

BREAKING CHANGE: bar

If they mean the same thing the ! in the example above is just a handy way to convey an existing property of the commit in the subject line -- a nice feature for sure. However, given that feat: foo and feat!: foo are explicitly not the same it would mean that the semantics of ! depend on the presence of BREAKING CHANGE.

If they don't mean the same thing then the semantics of the spec's features are a bit more consistent but you need to look at the whole commit message, or rely on tooling, to quickly identity breaking changes. Technically I guess you "need" to do that anyway if you don't control the commit messages but individual projects could require a ! indicator even with BREAKING CHANGE footers for the sake of the human reader.

@epage
Copy link
Author

epage commented Aug 7, 2023

I always saw ! as a way to make breaking changes standout in PR titles and one-line git logs

@ttd2089
Copy link

ttd2089 commented Aug 8, 2023

FWIW the reference implementation uses the last indication of a breaking change for the changelog notes which would agree with @epage's interpretation.

image

❯ node
Welcome to Node.js v19.2.0.
Type ".help" for more information.
> const {parser,toConventionalChangelogFormat} = require('@conventional-commits/parser');
undefined
> toConventionalChangelogFormat(parser("feat!: foo")).notes
[ { title: 'BREAKING CHANGE', text: 'foo' } ]
> toConventionalChangelogFormat(parser("feat!: foo\n\nBREAKING CHANGE: bar")).notes
[ { title: 'BREAKING CHANGE', text: 'bar' } ]
> toConventionalChangelogFormat(parser("feat!: foo\n\nBREAKING CHANGE: bar\nBREAKING CHANGE: baz")).notes
[ { title: 'BREAKING CHANGE', text: 'baz' } ]

@mentalisttraceur
Copy link

mentalisttraceur commented Aug 11, 2023

  1. I agree that if there is an explicit BREAKING CHANGE, it should take precedence over the top-level summary.

    I think that's the better design, and should be made explicit. I can try to drag up my logic for that from habitualized/subconscious/intuitive thought into words if anyone disagrees or doesn't find it obvious.

  2. I think it should be explicitly specified what happens if you have multiple breaking changes in one commit (this is an inevitability - no amount of "just keep changes separate" can prevent the possibility that one indivisible change causes two ripples for your promised interface which are worth calling out).

    I think that the correct choice here is to say that multiple BREAKING CHANGE comments in one commit get catenated (ideally with some way that preserves boundaries - I would just make an API which always contains a list of strings rather than possibly containing a string, but if things like the reference implementation want to keep backwards compat with the decision to just present it as a string, I'd at least join them with something that can't occur normally in a single BREAKING CHANGE entry... maybe two line feed characters).

    Failing that, the spec should warn, with prominent emphasis (bold, all caps, a different color, a visually distinct box, whatever) that if you as the user try to specify two or more breaking changes with two separate BREAKING CHANGE entry, all but one might be dropped by the implementation. But I would personally treat that behavior as a bug in the reference implementation, and any other implementation (was it even a consciously deliberate "we will drop all but the last occurrence of BREAKING CHANGES"? seems unlikely).

    Because any spec which permits dropping an explicit "BREAKING CHANGE" provided by the human user on the floor is clearly at best going to unexpected/surprisingly and silently throw away information that the user expected and intended to be used.

  3. I think there is no contradiction/problem with "the semantics of ! depend on the presence of BREAKING CHANGE." In fact, the semantics don't change, if we just frame it right:

    1. "BREAKING CHANGE: ..." indicates a breaking change and adds one breaking change description: "...".
    2. "!" indicates a breaking change without adding a breaking change description.
    3. If a commit has a breaking change and no breaking change descriptions, the first/summary line is used as the breaking change description.

@ttd2089
Copy link

ttd2089 commented Aug 15, 2023

  1. I think it should be explicitly specified what happens if you have multiple breaking changes in one commit...

This sounds 100% right to me.

  1. I think there is no contradiction/problem with "the semantics of ! depend on the presence of BREAKING CHANGE." In fact, the semantics don't change, if we just frame it right:

It's not necessarily a contradiction or a problem if the semantics of ! are conditional; it just adds a step to the process of interpreting the meaning of the subject line. Consider the following commit messages:

feat: foo

BREAKING CHANGE: bar
feat!: foo
feat!: foo

BREAKING CHANGE: bar

If the description following a ! is only a breaking change description when no other breaking change descriptions are present then the 2nd and 3rd subject lines are literally equivalent but semantically different while the 1st and 3rd subject lines are literally different but semantically equivalent. If ! unconditionally mean that the description in the subject line is a break change description then semantic equivalence corresponds to literal equivalence: the first is literally and semantically different from the 2nd and 3rd while the 2nd and 3rd are literally and semantically the same as each other.

The practical tradeoff seems to be that the conditional semantic allows the subject line to signal that there is a breaking change in the commit without requiring the description therein to describe the breaking change while the unconditional semantic allows for a breaking change in a subject description in a commit with other breaking changes.

E.g.

feat!: enable custom formatting for foo

BREAKING CHANGE: The previously-default formatting for foo must now be
explicitly specified.

Here, the conditional semantic allows us to use ! to call attention to the breaking change in the commit even though "enable custom formatting for foo" doesn't describe a breaking change because of the presence of the BREAKING CHANGE footer. The unconditional semantic would require us to remove the ! meaning we can't tell from the subject line alone that the commit has a breaking change.

feat!: validate remote TLS certs by default

Validate the remote certificate using the default ca-bundle whenever a TLS
connection is established unless an alternative callback is specified.

BREAKING CHANGE: Setting the certificate validation callback to `null`
explicitly will trigger the default validation. Use the `InsecureSkipVerify`
callback instead. 

Here, the unconditional semantic allows us to use the subject line to express the primary change as a breaking change and to add another breaking change description in the footer. The conditional semantic would require us to repeat the description from the subject line in another footer if we wanted both descriptions to appear as breaking changes.

I suppose the tradeoff could be avoided by using the conditional semantic for ! and having another way indicate that subject line description should be taken as a breaking change description even if another BREAKING CHANGE description is already present; though I'm not sure if there's a nice way to do that that's also backwards compatible.

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

No branches or pull requests

3 participants