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

Don't require formatting for commit titles, these cause more problems than they solve. #15

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

robert-ancell
Copy link

Adding the type/scope to the commit title is a bad idea for the following reasons:

Encoding data in the title indicates a problem in the tooling. If you need to clearly tag commits with information, then PR tags should probably be used. These are much more easily filtered in a UI.

Categories, while familiar to people, are generally a bad way of labelling data. You can fit the majority of cases into categories but there will always be cases where no category fits (which means either using no category, a general category or adding many infrequently used categories). Data also often falls into multiple categories. This can be handled by listing multiple categories for the data (i.e. treating as tags) or having to decide a threshold when a category is met. Due to these reasons picking categories is time consuming task without always a clear solution.

Having a code change, title and type/scope means you now have three things that need to be consistent. The risk of them not matching is increased over just having two.

A good analogy for this is code comments. The general modern consensus on comments is they should be used sparingly, and only in cases where the intention of the code can't be easily determined. If you have a comment that just literally states what the code does it is useless (this is the same for a PR title where the words match the type/scope). If you have to use a comment it is an indication that you should consider how your code is written. Rewriting the code often makes the comment redundant (the same for PR titles - if you need to add the type/scope then perhaps the title is not appropriately descriptive for the change).

With all the above issues there are likely to be many errors setting the type/scope which quickly undermines confidence in them. I have experienced this in many projects that use them.

… than they solve.

Adding the type/scope to the commit title is a bad idea for the following reasons:

Encoding data in the title indicates a problem in the tooling. If you need to clearly tag
commits with information, then PR tags should probably be used. These are much more easily
filtered in a UI.

Categories, while familiar to people, are generally a bad way of labelling data.
You can fit the majority of cases into categories but there will always be cases where
no category fits (which means either using no category, a general category or adding
many infrequently used categories). Data also often falls into multiple categories.
This can be handled by listing multiple categories for the data (i.e. treating as tags)
or having to decide a threshold when a category is met. Due to these reasons picking
categories is time consuming task without always a clear solution.

Having a code change, title and type/scope means you now have three things that need to be
consistent. The risk of them not matching is increased over just having two.

A good analogy for this is code comments. The general modern consensus on comments is they
should be used sparingly, and only in cases where the intention of the code can't be easily
determined. If you have a comment that just literally states what the code does it is useless
(this is the same for a PR title where the words match the type/scope). If you have to use
a comment it is an indication that you should consider how your code is written. Rewriting
the code often makes the comment redundant (the same for PR titles - if you need to add the
type/scope then perhaps the title is not appropriately descriptive for the change).

With all the above issues there are likely to be many errors setting the type/scope which
quickly undermines confidence in them. I have experienced this in many projects that use
them.
@robert-ancell robert-ancell marked this pull request as draft November 7, 2023 12:43
@robert-ancell
Copy link
Author

This PR is to give feedback (issues can't be opened), not intended to be merged as-is. If we did accept this change the document should probably have a section on how to write good PR titles.

@didrocks
Copy link
Collaborator

didrocks commented Nov 8, 2023

I agree that theis a big risk of wrong categorisation, which is one of the reason I excluded chore for instance, to have more specific ones.

Forcing the merge request having a story-like title is still a good thing, but the prefix undermines a little bit that effort.
We only started recently doing this in WSL and enterprise desktop projects and don’t see so many "maintenance" items so far, but I agree there are some risks associated here and you probably have more experience in projects following conventional commits than I have. I think @tim-hm has more experience here and can reflect his thoughts on this.

@tim-hm
Copy link
Contributor

tim-hm commented Nov 15, 2023

A few thoughts from me:

  • In my view tags should be reserved for snapshots / versions which is some meta data attached to a specific commit so they're not really applicable to describe the commit itself.
  • Also, I'm not sure comparing commit subjects and code comments is applicable either because they serve distinct/orthogonal purposes.
  • I agree there is a mismatch risk between types/scopes but contributing guidelines, automated checks and including a review of the commit subject/body in our PR process should address this. Wouldn't you want the commit text checked even if we didn't follow a convention commit style?
  • Personally I'm less fussed on a fixed list of types/scopes and I'd love to explore the approach that suits us. I think the snapd commit history is a great example of it being done well.
  • I think conventions are valuable and we shouldn't underestimate the benefits of a mostly-shared strategy across the team. For me this is one of the biggest reasons to adopt something like CCs.
  • Finally, I wonder if the type of project is important here when considering the utility of conventional commits ... I suspect its not but wanted to raise regardless.

My vote is to try using conventional commits (or similar) where it makes sense and see how we get on at the end of the cycle. Either way, looking forward to further refining our process!

@ashuntu
Copy link

ashuntu commented Nov 21, 2023

My personal opinion is that we should follow conventional commits (feat:, fix:, etc.) for all individual commits but not for PR titles or merge commits. Individual commits usually are just a single fix, feature, etc., while PRs may encompass many different fixes, features, etc.

Instead, I think PR titles/merge commits could follow something similar to snapd where the title prefix isn't feat: or fix:, but the more general area of the project the PR impacts, like interfaces/desktop:. I don't think it's useful to necessarily define a desktop-wide standard for PR titles, rather the projects themselves should decide what is useful to them or to not use a standard at all (in contrast, individual commits can always just follow standard conventional commit practices). I do think we could define a standard to at least not use the default "Merge pull request ### from foo" title for merge commits, though.

For example, in this PR: canonical/steam-snap#290 I didn't follow any set standard. Following what I've described, the PR title could be changed to something like scripts: Add script to detect NVIDIA 32-bit drivers, and the commit titles within could be changed to something like:

feat: Add script to check for 32-bit NVIDIA libraries
fix: Remove app listing
fix: Remove comments

Then, the merge commit title could match the PR title + the PR number: scripts: Add script to detect NVIDIA 32-bit drivers (#290).

I think this helps scope commits and PRs, and encourages committing often. Plus, it makes scanning through PR titles and commit titles much easier, while also providing additional ways to search for commits or PRs.

@didrocks
Copy link
Collaborator

didrocks commented Nov 22, 2023

I guess the issue with your example is what you put in the title itself. scripts: Add script to detect NVIDIA 32-bit drivers is a description of a bad commit title. The title is not about what you did technically but what is the intent. Also "scripts:" is clearly not a fonctionality by itself. At best, it could be a domain but I don’t think it should or help clarifying the story by itself (as an user, do I care about this being a script, a binary, a module?)

I would rephrase this particular case to feat: detect NVIDIA 32-bit drivers for instance.

Same for

fix: Remove app listing
fix: Remove comments

Here, you are describing the what and not the why. It’s basically a tautology of the diff that I can directly open up. Those kinds of content is better in the long commit message by itself. Arguably in this example, how is "Remove comments" a fix?

There are multiple existing writeup on this, like https://www.gitkraken.com/learn/git/best-practices/git-commit-message.

The idea is that you care about the story, the story is the merge commit, this is what you try to present at the higher level of abstraction. One PR is also one idea (but not necessarily one commit). If you can’t summarize one PR under a single umbrella, it means that your PR is too large and should be broken down.

The smaller units are how you got to that story, they don’t not necessarily pass existing tests. Also, the longer commit description helps when doing git blame to understand why we went there.

Basically, you end up with something like:
Merge commit: feat: feature foo
which is composed of those commits:

implement foo support # the long message can be ("modify bar to inject foo"). Note that this commit can break existing tests
fix existing tests for foo
package tests for foo
refresh golden files

I don’t think that adding feat: or fix to any of those 4 commits makes sense, because the tests are temporary broken, and any bissection should be done only on the merge commits, which are the ones where all tests should passed, checked by CI and so on. This will auto-generate also changelog with a too fine-grained granularity with no good content for the end user, which is the only benefit I can find that is shared widely in the developer community on using those conventional commits.

I hope that makes sense.

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.

4 participants