Enforcing 50 characters of the initial commit line

Hello,

I've filed a PR to our PR processor to enforce maximum length of the
initial commit line to 50 characters which is the golden standard. Most
editors warn you if you exceed this limit (at least Vim :slight_smile: and it's
easy it create local git hook to prevent you from local commits so you
don't get surprised.

This is in comply with our Coding Standards:

While we recommend 50 characters and this is what PR processor target
for in my patch, we can discuss and maybe move this limit up a bit. If
you dislike this, speak up now or never.

··· -- Later, Lukas #lzap Zapletal

Hi,
I think the main problem with the 50 char limit is that we already lose 15
chars from that to the "Fixes #12345 - " format, leaving very little room
to describe the change.
I would say make the limit 65 or change the format.

··· On Mon, Apr 11, 2016 at 4:24 PM, Lukas Zapletal wrote:

Hello,

I’ve filed a PR to our PR processor to enforce maximum length of the
initial commit line to 50 characters which is the golden standard. Most
editors warn you if you exceed this limit (at least Vim :slight_smile: and it’s
easy it create local git hook to prevent you from local commits so you
don’t get surprised.

https://github.com/theforeman/prprocessor/pull/36

This is in comply with our Coding Standards:

Foreman :: Contribute

While we recommend 50 characters and this is what PR processor target
for in my patch, we can discuss and maybe move this limit up a bit. If
you dislike this, speak up now or never.


Later,
Lukas #lzap Zapletal


You received this message because you are subscribed to the Google Groups
"foreman-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


Have a nice day,
Tomer Brisker
Red Hat Engineering

I've been told to put the BZ title as my one squashed commit message,
rather than what the commit actually does, like so: "Fixes #14147 -
hammer content-view filter create ignores repos if name used instead
of ID". The length is then dependent on the Bugzilla title and is
often longer than standard commits.

··· On Mon, Apr 11, 2016 at 10:11 AM, Ewoud Kohl van Wijngaarden wrote: > On Mon, Apr 11, 2016 at 03:37:25PM +0200, Lukas Zapletal wrote: >> > I would say make the limit 65 or change the format. >> >> I agree with that, that's a decent starting point. >> >> I also reworked my prprocessor patch to also watch for 72 line breaks to >> comply fully with the 50/72 rule we aim for in our guide. > > The 72 should be a warning, not a hard requirement. Sometimes you want > to include a few log messages without formatting them. In that case you > give up readability which goes against the goal of the 72-limit. > > -- > You received this message because you are subscribed to the Google Groups "foreman-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an email to foreman-dev+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout.


Andrew Kofink
akofink@redhat.com

I would actually advise against this. BZ titles don’t necessarily make good
commit descriptions as they describe the problem but not the actual work
done to fix the problem. I’d rather see something like “Fixed content_view
permission for non-admins” vs “Can’t publish content view”.

David

··· On Mon, Apr 11, 2016 at 10:27 AM, Andrew Kofink wrote:

I’ve been told to put the BZ title as my one squashed commit message,
rather than what the commit actually does, like so: “Fixes #14147 -
hammer content-view filter create ignores repos if name used instead
of ID”. The length is then dependent on the Bugzilla title and is
often longer than standard commits.

On Mon, Apr 11, 2016 at 10:11 AM, Ewoud Kohl van Wijngaarden > ewoud@kohlvanwijngaarden.nl wrote:

On Mon, Apr 11, 2016 at 03:37:25PM +0200, Lukas Zapletal wrote:

I would say make the limit 65 or change the format.

I agree with that, that’s a decent starting point.

I also reworked my prprocessor patch to also watch for 72 line breaks to
comply fully with the 50/72 rule we aim for in our guide.

The 72 should be a warning, not a hard requirement. Sometimes you want
to include a few log messages without formatting them. In that case you
give up readability which goes against the goal of the 72-limit.


You received this message because you are subscribed to the Google
Groups “foreman-dev” group.
To unsubscribe from this group and stop receiving emails from it, send
an email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


Andrew Kofink
akofink@redhat.com


You received this message because you are subscribed to the Google Groups
"foreman-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

> I would say make the limit 65 or change the format.

I agree with that, that's a decent starting point.

I also reworked my prprocessor patch to also watch for 72 line breaks to
comply fully with the 50/72 rule we aim for in our guide.

··· -- Later, Lukas #lzap Zapletal

The 72 should be a warning, not a hard requirement. Sometimes you want
to include a few log messages without formatting them. In that case you
give up readability which goes against the goal of the 72-limit.

··· On Mon, Apr 11, 2016 at 03:37:25PM +0200, Lukas Zapletal wrote: > > I would say make the limit 65 or change the format. > > I agree with that, that's a decent starting point. > > I also reworked my prprocessor patch to also watch for 72 line breaks to > comply fully with the 50/72 rule we aim for in our guide.

I agree with David on this.

··· On Mon, Apr 11, 2016 at 11:21:28AM -0400, David Davis wrote: > I would actually advise against this. BZ titles don’t necessarily make good > commit descriptions as they describe the problem but not the actual work > done to fix the problem. I’d rather see something like “Fixed content_view > permission for non-admins” vs “Can’t publish content view”. > > > David > > On Mon, Apr 11, 2016 at 10:27 AM, Andrew Kofink wrote: > > > I've been told to put the BZ title as my one squashed commit message, > > rather than what the commit actually does, like so: "Fixes #14147 - > > hammer content-view filter create ignores repos if name used instead > > of ID". The length is then dependent on the Bugzilla title and is > > often longer than standard commits.

> The 72 should be a warning, not a hard requirement. Sometimes you want
> to include a few log messages without formatting them. In that case you
> give up readability which goes against the goal of the 72-limit.

Indeed, it is.

··· -- Later, Lukas #lzap Zapletal

+1 - commit messages should always be a description of the change, written
in the imperative (
http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html via
Foreman :: Contribute for some light reading
:P)

What David says is spot on - and this is especially true when you consider
that a bug may have several potential causes, so a description of the bug
doesn't help you know (from the commit message) which cause you fixed.