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

BREAK: feat: indicate breaking changes in header via "BREAK: " prefix #64

Closed
wants to merge 1 commit into from

Conversation

apetro
Copy link
Contributor

@apetro apetro commented Jul 5, 2018

BREAKING CHANGE: adds new "BREAK: " prefix before type in header in commits including breaking API changes. Tools written to the spec before this feature may fail to parse or flag as non-compliant commit messages using this feature. Breaking change commit messages written to prior versions of this spec will not be compliant with versions of the spec with this change.

Tools can regain spec compliance by adding support for parsing the new "BREAK: " prefix.

For a commit that includes API-breaking changes, that fact is the most important thing to say about the commit, so lead with it in the commit message header. Indicating breakingness in the commit message header makes API breakages more apparent on skim in more contexts.

BREAKING CHANGE: adds new "BREAK: " prefix before type in header in commits including breaking API changes. Tools written to the spec before this feature may fail to parse or flag as non-compliant commit messages using this feature. Breaking change commit messages written to prior versions of this spec will not be compliant with versions of the spec with this change.

Tools can regain spec compliance by adding support for parsing the new "BREAK: " prefix.

For a commit that includes API-breaking changes, that fact is the most important thing to say about the commit, so lead with it in the commit message header. Indicating breakingness in the commit message header makes API breakages more apparent on skim in more contexts.
@apetro
Copy link
Contributor Author

apetro commented Jul 5, 2018

Alternative solution to #63 or #43 .

Using "BREAK:" might be better than a sigil (#43) in that

  • "BREAK:" may be more readily understandable by a reader less familiar with the spec (whereas one would need to learn the meaning of the sigil).

Using "BREAK:" might be better than tweaking the capitalization of the type (#63) in that

  • "BREAK:" may be more readily understandable by a reader less familiar with the spec (whereas one would need to learn the meaning of different type capitalizations).
  • developers may not always be taking care with that capitalization. To the extent that developers inadvertently use ALL-CAPS types, those will be false signals of API breakage.
  • It may be desirable for capitalization not to have semantic meaning in the commit messages.

Using "BREAK:" does consume 6 characters of the header, however.

I'm not sure what the right solution is here. I do think the header should reflect breakingness, rather than breakingness only being reflected in the body or footer.

@damianopetrungaro
Copy link
Member

damianopetrungaro commented Jul 5, 2018

Hey @apetro, thank you for the time you spent trying to improve the specs 😄

Unfortunately, open a PR with some changes without discussing them before it's not a good choice.
If you open an issue and try to discuss your changes you'll receive feedback on it before starting writing it 😄
By the way, I do not agree with the changes you proposed.

There's already a way to specify breaking changes and I do not see how it the use of CAPS commit types may improve the specs, instead, it may create confusion and false positive.

As you can see number #43 try to apply something similar, in a more explicit way.
Feel free to discuss it on the issue or open a new one and link it to #43.
Thank you for the time you spent on it, I really appreciate the initiative and feel free to open all the issue you want 😄

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.

2 participants