-
Notifications
You must be signed in to change notification settings - Fork 558
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
Conversation
Love it! |
For context, the Ruby programming language has a concept of bang methods, wherein method names ending in I've also found it cumbersome that the existing SemVer mechanism for a breaking change requires a All to say, yes! I welcome this addition with open arms. 🙌 |
@zeke 👍 glad for the support, are you okay with the fact that at the outset we would require that |
@@ -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 |
There was a problem hiding this comment.
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 ☝️
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bcoe ☝️
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this 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!
@bcoe I think that allowing |
I'm personally fine with having a |
@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. |
@damianopetrungaro @zeke I had thought on this a little bit ... if there was only |
That makes sense to me. |
@bcoe please add an example and for me it's an amazing 👍 |
This PR is not published to https://www.conventionalcommits.org/ yet, right? |
… header This was introduced in conventional-commits 1.0.0-beta.4 conventional-commits/conventionalcommits.org#134
Hate it. I think a I fundamentally disagree with that - here's part of what I'm thinking. Sadly, I missed that... ;/
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.
Yes, of course, you will do that anyway. As about "machine-readable" and grep-ing, heck, you can do that now anyway, grep for Okay, accepted and merged - I and my parser will adhere to that, but never make that |
@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 What are your main ideas about it? Info:
Or |
I understand and make sense, that's why I propose to include And yes, it's easy to detect it. I'm the author of I just fundamentally disagree making it required and in future making |
@tunnckoCore feel free to open a ticket and let's discuss the There are a lot widely used packages that are supporting conventional-commits and we are tracking also their opinion about it.
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. |
It's not an ultimatum, I'm just saying because I know that I (or a single person) can change the decisions.
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. |
so, I wanted to chime in that we recently landed this: which should probably be released as |
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:
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