Hey all, a few years ago, there was a discussion around adding checks for line length in commit messages in foreman-bot, which was added here to be 65 for commit titles and 72 for commit body
These checks have been really good at keeping commit titles from being overly-descriptive, but sometimes can feel a little too restricting.
If I have a commit that begins Fixes #12345 - Content View Version, I’m already at 35 characters, this doesn’t allow for much more to be written. A side effect of this is you see a lot of abbreviations like CVV in the commit messages, which may make sense to us, but is not as beginner and community friendly.
I also like to include links in the commit message, this can be hard when the link is long.
I’m not sure where exactly github wraps the lines in pull requests, but it seems much longer than 65/72.
The change is IMHO for the worse. Anything longer than 55 characters is simply too much of information. Yes, it’s a challenge but that’s why there’s the body which explains it.
My opinion on wrapping lines at 7x-th does not change for decades. I prefer hard wraps and I will continue doing so. This is not only about (legacy?) 80x25 terminal. We have 4K+ monitors and web pages are still slim noodles - it’s because text become unreadable when lines are longer than 100-150 characters. People skip lines when returning from the right to left (or vice versa for some languages).
I think it is completely fine to have guidelines around 55/65 characters for a commit title and 72 for the commit body. I’m in favor of keeping these limits in our style guideline.
I totally get the point about runaway text that doesn’t wrap. I know we have all seen crazy things happen when we resize our terminals on our fancy widescreen monitors, I think it’s great we have guidelines around this and automated checking with foreman-bot.
I’m asking we change what is strictly enforced by foreman-bot. The guidelines can stay the same. My feeling is the current hard limit can get in the way of communicating information, that is my main motivation for proposing the change. Some things I’ve seen that I think would improve with more lenient limits:
Unnecessary use of acronyms that are only understood by a certain section of developers
Not as much information is included in the actual commit message, but rather the edited PR message. This includes things like links, error messages, reproducing steps, copypastas from other sources
Confusion from first-time contributors and community members on the foreman-bot messages in PRs
I know other communities have limits that are similar to what we have currently, but I think we should do what works best for us overall. We work on a product that has something called a “Content View Puppet Environment” (31 chars) and we include issue information in every commit title, which leads me to believe a more lenient commit message length would be beneficial.
Just a bit of history on this point - when we started enforcing the style, we decided to make the title 65 and not 50 as is usually recommended precisely because of the Fixes #12345 - part which added 15 characters to the title.
Some background on why we chose these (and generally good tips for writing good commit messages) case be found here and here.
While the bot will yell at you for longer titles, it doesn’t actually block merging PRs that have longer titles - but I would argue it is better to keep these guidelines. Using an acronym in the title and expanding on it in the body isn’t a problem imho - the problem is when people try to cram everything into the title and leave the body empty. Also, possibly having objects with long confusing names such as “Content View Puppet Environment” (even I’m not sure what that is, and I’ve been working on the project for almost 5 years) is a problem of its own.
Softlimits are useless. Warnings are useless. It’s the errors which forces programmers to do actions Now on a serious note.
I am opinionated on the commit message being enforced at 65nd character for multiple reasons. However I understand that enforcing 72nd column on the commit message can be limiting for the purposes you describe (long link, error message). This can be very useful.
I don’t understand your example with Content View Puppet Environment, the moment you put this in the commit title completely nullifies the rest of the title because it will be:
hidden
wrapped
forgotten forever
I simply think that this
Fixes #12345 - promoting CVPE is now properly logged
The application was not sending any logs about Content View
Puppet Environment and this is now fixed.
is better than this
Fixes #12345 - promoting Content View Puppet Environment is now properly logged
Therefore I propose to keep 65 for titles and stop enforcing commit message line length completely.
This allows anybody to make own choice of hard wrapping commit messages or not. Of course for links or error messages it makes a lot of sense not to do it.
By the way, the bot does ignore properly github-formatted lines which start with 4 whitespace characters, however there are other markup possibilities which will be ignored by the bot - thus the confusion I believe.
By the way, my proposal is not in the poll from the very beginning, for this reason whatever gets voted is not a fair winner.
Correct the create of multiple dhcp entry for one host.
See the discussion in https://community.theforeman.org/t/foreman-1-20-host-creation-fails-to-provision-dhcp-entry/12879/15
Observations:
The title is just one character off our hard limit once you add Fixes #12345 prefix, easy fix, just one character extra.
The body is just a link to discussion, honestly I don’t actually like that - copy pasting or better writing a good explanation is always better than link.
Solution to that is formatting the link properly which I agree can be confusing to newcomers (*)
The bot errors out due to two reasons:
Missing the Fixes prefix
Text is longer than 72 columns
(*) this is how it passes:
Fixes #12345 - correct the create of multiple dhcp entry for one host
See the discussion in
https://community.theforeman.org/t/foreman-1-20-host-creation-fails-to-provision-dhcp-entry/12879/15
This sounds fair, the main thing I think most of us fight with is the commit body itself. I did the poll as a way to gauge interest, realizing that I couldn’t get every option in there and we would have to have a discussion to refine things a bit more (as i’m glad we are!)
It seems the main point of contention is the commit title itself, which makes sense, this was one of the large motivators of the change. Could we do 72 characters for the commit title? The blog post everyone keeps linking literally says So shoot for 50 characters, but consider 72 the hard limit.
Even despite that, I really think we should do what works best for us, not what blogs say. My point with the Content View Puppet Environments is that we work on a product that isn’t simple, I’ve found even trying to keep my commit titles concise I’ll run out of room. I’m willing to do another poll, but I’m curious to hear everyone’s opinion, it could just be me having this issue
lzap:
Therefore I *propose to keep 65 for titles* and *stop enforcing*
commit message *line length* completely.
This sounds fair, the main thing I think most of us fight with is the
commit body itself. I did the poll as a way to gauge interest,
realizing that I couldn’t get every option in there and we would have
to have a discussion to refine things a bit more (as i’m glad we are!)
It seems the main point of contention is the commit title itself,
which makes sense, this was one of the large motivators of the change.
Could we do 72 characters for the commit title? The blog post everyone
keeps linking literally says |So shoot for 50 characters, but consider
72 the hard limit.|
Which i think is similar to what we have today. The foreman-bot fails
on 65 but its up to the reviewer to make a judgement call if the commit
message is too long but justifiable. I’ve merged PRs in the past that
exceeded the limit because the committer and myself agreed that it
needed to be longer than 65 characters. This should be rare IMO, ~1% of
prs.
Maybe adjusting foreman-bot to throw a different message if it breaks an
official hard limit would be helpful?
Doesn’t it keep repeating the message every time you push up new changes? This is the hard part of it being optional, it is only optional if you don’t mind foreman-bot messages every time you push to your PR branch.
Absolutely, I should state that I linked that article for the reasoning rather than what he/she proposes as the limits.
In our case, we actually force more than 50 because of the Fixes #12345 - prefix so we customized this to our needs. Frankly I use git log or tig quite a lot on terminals and let me show you a pic:
This is tig on a terminal with 127 columns. I completely get that these days almost nobody uses 80x25 (although I respect that some might still use this format), but you can see that our limiting still makes the whole log history readable on reasonably sized widths.
I think it is still useful. Why don’t we start with removing enforcing wrapping in the body and return back to the title length after few months. If folks feel the workflow improved maybe the title length won’t be an issue anymore.
This sounds like a good change. How about we keep the limit, but make it a way more liberal one (I’m thinking 100 chars) to prevent horribly unformated commits?
I can open a PR if there is no issue with changing or removing the commit body line limit.
Now on a serious note.
