Repository navigation
Decision required: Change commit messages to "conventional commits" #3054
Description
Activity
I am in favor of this.
I would like to continue putting the path (i.e. in conventional commit terminology it is scope) into the commit message, e.g.
swarm,protocols/kadortransports/tcp.I would like to continue putting the path (i.e. in conventional commit terminology it is scope) into the commit message, e.g.
swarm,protocols/kadortransports/tcp.At least
release-pleasewill figure out what versions to bump based on the touched files so the scope is only informational.Is there much value in adding the scope? It often takes up a significant portion of the commit message title, which makes it hard to keep under 50 characters (that is the recommended length).
Can we loosen this to a "we MAY add the scope"? I'd personally prefer not to, esp. when multiple things are being touched. GitHub's file free makes it quite to see what is being touched and I am sure there are funky Git one-liners too!
👍 In favor of conventional commit messages.
Is there much value in adding the scope? It often takes up a significant portion of the commit message title, which makes it hard to keep under 50 characters (that is the recommended length).
Can we loosen this to a "we MAY add the scope"? I'd personally prefer not to, esp. when multiple things are being touched. GitHub's file free makes it quite to see what is being touched and I am sure there are funky Git one-liners too!
Agreed. I stopped used full paths because of those reasons. In favor of optional scopes where it makes sense.
Thanks everyone, I'll consider this change to be approved unless someone objects by Friday, Nov 11th.
No objections on my end.
Thanks everyone, I'll consider this change to be approved unless someone objects by Friday, Nov 11th.
I suggest updating https://github.com/libp2p/rust-libp2p/blob/master/docs/coding-guidelines.md accordingly and consider this the new convention once merged.
Is there much value in adding the scope?
It does help me to quickly understand the context of a pull request. A couple of examples:
-
Use Description for commit message Use
Descriptionfor commit message #3082Which description? A simple
.githubwould have helped. -
Avoid collecting all RecordStore records into a Vec in the behaviour Avoid collecting all RecordStore records into a Vec in the behaviour #3021
Which
RecordStore? In our Kademlia implementation? In Gossipsub? -
Use Multiaddr instead of ConnectedPoint in DialError::WrongPeerId Use
Multiaddrinstead ofConnectedPointinDialError::WrongPeerId#2793Is this in Swarm? Is this in core? Is this the usage of DialError in one of the behaviours?
In favor of optional scopes where it makes sense.
Sounds like a good middle ground.
-
I suggest updating https://github.com/libp2p/rust-libp2p/blob/master/docs/coding-guidelines.md accordingly and consider this the new convention once merged.
Where in that document would you like to see this documented?
I was thinking of enforcing this via mergify: #3083
with #3165 should we re-think adding documentation?
I'd suggest to use GitHub actions instead. There should be plenty of them out there that either directly validate conventional commits or let you define a regex for the title.
Worst case we use bash :)
Reacted by João Oliveira
I'd like to propose to change our commit message convention to "conventional commits": https://www.conventionalcommits.org/en/v1.0.0/
The main driver for this is automation in the form of automated changelogs and automated version bumps. See #2902.
In particular, tools like https://github.com/googleapis/release-please can do this but there are many others and I'd like to make this decision independent of which tool we are going to use.
At the moment, the convention for our commit messages is:
With conventional commits, we would change this to:
feat,fix,chore,testetcSee some examples here: https://www.conventionalcommits.org/en/v1.0.0/#examples
I benefits I see are pretty much the ones mentioned in the specification: https://www.conventionalcommits.org/en/v1.0.0/#why-use-conventional-commits
Something that works really well with our current workflow is the squash-merging. It means we don't have to educate users in how to write commit messages but just edit the titles of their PRs to match our conventions. That is not very hard and something one of us can do as we review it. The only important bit is that we need to review that title (and whether the change is breaking) before merging. I'd suggest we use the
!instead ofBREAKING CHANGEto identify those.There are also some downsides to conventional commits:
There is an argument that changelogs should be written manually and not auto-generated. The argument is mostly that changelogs are for humans and thus should be written with priority and clarity in mind, something that is hard to do in an automated fashion. There is more criticism here: https://common-changelog.org/#42-conventional-commits
I think the provided criticism is somewhat valid but can also be dealt with:
release-pleasedifferentiates between e.g. betweenchoreandfeatcommits.chorePRs will not show up in the changelog. This avoids the "noise" problem.@mxinden @jxs @elenaf9 Please voice your opinion on this topic here :)