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: introduce ! for indicating breaking changes in header #134

Merged
merged 2 commits into from
Apr 10, 2019

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Mar 23, 2019

Based on discussion in #43 I've made an effort to describe a new ! character, which can optionally be used in the header to indicate a breaking change.

The value I see in making this optional:

  • the feature will be available for folks who would like to use it, but the spec doesn't drift away from angular's commit message format.
  • tooling can more gradually move over to this new semantic construct.

I propose we wait and see how widely ! is adopted, and consider making it a MUST in the future if it's seeing wide adoption.

CC: @DominicKramer, @apetro, @ofrobots

@zeke
Copy link
Member

zeke commented Mar 23, 2019

Love it!

@zeke
Copy link
Member

zeke commented Mar 24, 2019

For context, the Ruby programming language has a concept of bang methods, wherein method names ending in ! are conventionally known to perform some kind of permanent operation, like updating a database, writing to a file, etc. I've also enjoyed the brevity and expressiveness of these methods.

I've also found it cumbersome that the existing SemVer mechanism for a breaking change requires a BREAKING CHANGE string within the the "body" (i.e. not the first line) of a git commit message. This not only requires more work or deviation from normal one-liner workflows when writing commit messages, it also requires educating newcomers to the semantic release world about this most idiosyncratic way to do a major version bump.

All to say, yes! I welcome this addition with open arms. 🙌

@bcoe
Copy link
Member Author

bcoe commented Mar 25, 2019

@zeke 👍 glad for the support, are you okay with the fact that at the outset we would require that ! requires a BREAKING CHANGE: description either in the body or footer? otherwise my concern is that we don't communicate enough information in the commit history about what about the change was breaking.

@@ -88,6 +88,8 @@ per-line.
1. A description MUST be provided after the `BREAKING CHANGE: `, describing what has changed about the API, e.g., _BREAKING CHANGE: environment variables now take precedence over config files._
1. Types other than `feat` and `fix` MAY be used in your commit messages.
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. A `!` MAY be appended prior to the `:` in the type/scope prefix, to further draw attention to breaking changes. `BREAKING CHANGE: description` MUST also be included in the body
Copy link
Member

Choose a reason for hiding this comment

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

We may add an example too for this ☝️

Choose a reason for hiding this comment

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

along with the sounds confusing to me. It may imply lexical adjacency. How about something like:

or footer, when using the ! prefix.

Choose a reason for hiding this comment

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

I think or footer when using the ! prefix. (without a comma) sounds good. I also think just ending the sentence at or footer. is good too.

Copy link
Contributor

@apetro apetro Mar 26, 2019

Choose a reason for hiding this comment

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

Whatever the in-the-title signifier is for a breaking change, I'd strongly prefer that Conventional Commits specify it as a MUST rather than a MAY. I'd like to be able to reliably notice breaking changes when scanning only the first lines of commit messages. MAY makes it unreliable.

If it's a MAY: Skimming some changes, I see no signifier in the commit message titles. Oh good, no breaking changes. Except I don't know that. I still need to look at the full body and footer of all the messages in order to reliably notice all the breaking changes documented by commit messages compliant with Conventional Commits.

If it's a MUST: Skimming some changes, piping their titles through grep even, I see no signifier in the commit message titles. Oh good, no breaking changes.

Copy link
Member

Choose a reason for hiding this comment

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

@apetro this is a MAY it's because we don't want to introduce a BC across all the tools for something that still in a "test" phase.

The MUST has always been the BREAKING CHANGE: in the message body.
If the ! will be adopted by most of the community then we may think about doing it mandatory.

Copy link
Contributor

@apetro apetro Mar 29, 2019

Choose a reason for hiding this comment

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

Conventional Commits is currently v1.0.0-beta.3. Beta is the opportunity to include the features that the spec needs to be effective. Yes, MUST-ing the sigil is a breaking change, but the time to make a breaking change to this spec is before 1.0.0.

I feel quite strongly that it should be a MUST and that MUST makes the feature a whole lot more valuable than does MAY. Conventional Commits is "unfit for purpose" without MUST-ing an indication of breaking changes in the commit title. (I mean "unfit for purpose" here as a term of art, not as a pejorative.) If a commit includes a breaking change, that is surely the very most important thing to say about that commit, and so the spec should require leading with that, in the commit title, in a predictable, reliable, machine-detectable way, and not allow burying the lede.

Because I believe this spec feature is absolutely essential, I believe it's worth breaking all the existing tools to achieve.

That said, if I can't get you to MUST, maybe it could at least be a SHOULD ?

Copy link
Member

Choose a reason for hiding this comment

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

@bcoe ☝️

Copy link
Member Author

Choose a reason for hiding this comment

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

@apetro I'm less convinced on this convention being a necessity (although I bet there's a chance my opinion shifts in the future); I'm hesitant to enforce additional ritual in the spec at this time.

however, I would (personally) be happy to opt for the language SHOULD rather than MUST. How do other folks feel (@hutson, @damianopetrungaro, @zeke, @ofrobots).

Copy link
Member

Choose a reason for hiding this comment

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

I preferiti SHOULD as already explained.
I do not see the necessary of a BC at this stage of the addition.

If in the future tools and ppl will use it then use MUST will be there :)

Copy link

Choose a reason for hiding this comment

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

SHOULD sounds like a good compromise to me.

Copy link
Contributor

@hutson hutson left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@zeke
Copy link
Member

zeke commented Mar 26, 2019

are you okay with the fact that at the outset we would require that ! requires a BREAKING CHANGE: description either in the body or footer?

@bcoe I think that allowing ! as an alternative rather than a supplement to BREAKING CHANGE would be ideal in terms of ease of use, but that's a big change. What you have here seems like an incremental step to eventually possibly enabling that behavior.

@DominicKramer
Copy link

I'm personally fine with having a ! sigil and a BREAKING CHANGE: statement in the body since I can still see from the first line of a commit that it is a breaking change.

@damianopetrungaro
Copy link
Member

damianopetrungaro commented Mar 26, 2019

@zeke The discussion started because we need a easy way (reading the first line of the log) to understand where a BC has been introduced.

Anyway you'll need to explain what BC you introduced.

@bcoe
Copy link
Member Author

bcoe commented Mar 28, 2019

@damianopetrungaro @zeke I had thought on this a little bit ... if there was only ! you could imagine that the description itself is taken to be the BREAKING CHANGE; but I think we shouldn't jump to this yet.

@zeke
Copy link
Member

zeke commented Mar 28, 2019

the description itself is taken to be the BREAKING CHANGE

That makes sense to me.

@damianopetrungaro
Copy link
Member

damianopetrungaro commented Mar 29, 2019

@bcoe please add an example and for me it's an amazing 👍

@bcoe bcoe merged commit 0a97786 into master Apr 10, 2019
@bcoe bcoe deleted the introduce-bang branch April 10, 2019 23:01
@mrcasals
Copy link

This PR is not published to https://www.conventionalcommits.org/ yet, right?

@bcoe
Copy link
Member Author

bcoe commented Apr 15, 2019

@mrcasals see #141

@tunnckoCore
Copy link
Member

tunnckoCore commented Jun 20, 2019

Hate it. I think a breaking type (as seen in parse-commit-message) may be a better and more visible way to introduce that.

I fundamentally disagree with that - here's part of what I'm thinking. Sadly, I missed that... ;/

@zeke: This not only requires more work or deviation from normal one-liner workflows when writing commit messages

That's the important part. You must be careful and pay attention when making this. Also, when breaking change is introduced you will/should/must anyway write a body describing it.

@apetro: I'd like to be able to reliably notice breaking changes when scanning only the first lines of commit messages. MAY makes it unreliable.

@apetro: I still need to look at the full body and footer of all the messages in order to reliably notice

Yes, of course, you will do that anyway.

As about "machine-readable" and grep-ing, heck, you can do that now anyway, grep for BREAKING CHANGE.


Okay, accepted and merged - I and my parser will adhere to that, but never make that MUST or BREAKING CHANGE optional.

@damianopetrungaro
Copy link
Member

@tunnckoCore I agree with you that it is really important to have a description of why there is a breaking change and what changed.

Before introducing a BC in the specs we need to investigate and see if the community like it or not, for the moment (at least I think) you are the first one that doesn;t like it, and we need to work together understand where are the possible issues and find a good trade-off so that the community will still use it.

The main topic is that "I want to see where the BC has been introduced just reading the first line of the commit message." because a lot of people use aliases to see only the first line of the commit message.

So an easy way is to introduce a ! in the first part before the : (so it is easy to catch using regex).

What are your main ideas about it?


Info:

@zeke The discussion started because we need a easy way (reading the first line of the log) to understand where a BC has been introduced.
Anyway you'll need to explain what BC you introduced.

Or

#134 (comment)

@tunnckoCore
Copy link
Member

tunnckoCore commented Jun 20, 2019

I understand and make sense, that's why I propose to include breaking type to the spec instead of !. It would be better bet.

And yes, it's easy to detect it. I'm the author of parse-commit-message and it will be easy to add !? in this regex here.

I just fundamentally disagree making it required and in future making BREAKING CHANGE: optional. I won't update the parser to such future updates of the spec.

@damianopetrungaro
Copy link
Member

@tunnckoCore feel free to open a ticket and let's discuss the breaking type (but I disagree with it, we already have different ways to handle the BC detection).

There are a lot widely used packages that are supporting conventional-commits and we are tracking also their opinion about it.


I won't update the parse to such future updates of the spec.

I'm sorry but I strongly disagree with the approach of "or we do this or I am out" because this is not the way that OS software is built.
Being in a community means discussion and help us with each other also if there's something and we don't like.
I am expecting this from you as I am from all the other maintainers of other tools.

@tunnckoCore
Copy link
Member

"or we do this or I am out"

It's not an ultimatum, I'm just saying because I know that I (or a single person) can change the decisions.

because this is not the way that OS software is built.

Yes of course, open source is built on community, but when you can you should take a stand. And there's nothing bad at that. :)

I won't debate more, we are clear :) I just wanted to share, a bit late, my thoughts on the topic just as a community member and author.

@bcoe
Copy link
Member Author

bcoe commented Jun 24, 2019

so, I wanted to chime in that we recently landed this:

df6de82

which should probably be released as beta.5 of the spec. I've been finding, in the wild, that I much prefer using ! in isolation, without BREAKING CHANGE.

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

Successfully merging this pull request may close these issues.

10 participants