7 ms·

One thing I disagree with is: > I wouldn’t expect all commits (especially ones of this size) to have this level of detail. (emphasis added) - actually in my e

by OJFord 3y ago

One thing I disagree with is: > I wouldn’t expect all commits (especially ones of this size) to have this level of detail. (emphasis added) - actually in my experience it's often the little ones, innocuous looking things that might really need a relatively longer explanation. Yesterday I wrote three paragraphs on why I added `--limit=999` to a `gh pr list` because it's confusing: there's already a `limit(` in the `--jq` argument, and the higher it is (given say infinite PRs in total) the lower the end result will actually be. (Yes I wrote a comment too. And probably spent even longer thinking about and working it up than writing about it; hopefully I'll recall it as an example the next time someone implies the job is about churning out code!)

pdpi 3y ago
I find myself commenting code in a similar pattern: A small kernel of "interesting" code that has a 1:1 ratio (or higher) of comments to code, which enables the rest of the codebase to be "boring" self-documenting boilerplate-y code that doesn’t really warrant much in the way of commenting.
keybored 3y ago
For a commit that adds a language binding (and might be 100+ additions/deletions) I might just say “Add X function”. Because I’m just following established patterns. But for the linked kind of change? Yeah, several paragraphs of explanation is definitely useful.
unregistereddev 3y ago
I agree with you that the little innocuous things often need a longer explanation, but the linked commit message is way too long IMO. It either wastes the readers' time, or it causes the readers' eyes to gloss over at the wall of text. You don't need to document your entire journey in order to document your findings and explain why. > This was a non-ascii whitespace character that caused `ArgumentError: invalid byte sequence in US-ASCII` when running `bundle exec rake` ^ should be sufficient. It includes enough keywords to come up in a search if someone has a similar problem in the future, it contains the root cause of the problem, and it is short enough that people are unlikely to gloss over it.
- mbork_pl 3y ago
  The article explains why all the rest is, maybe not needed, but good to have.
- OJFord 3y ago
  Yes, it's not my preferred style either, but it's much better than 'fixes error' type thing, subject line only, that's so common. I like the form: Fix ArgumentError 'invalid byte sequence' Non-ASCII whitespace characters cause [...]. This was apparent in [...] because [...]. This commit fixes the issue by removing the offending character; so the file is now solely ASCII characters. Or that sort of thing. Subject tells me why, body tells me what the problem was and how it was fixed. (Who, when, where are already in the commit metadata! The diff shows a very literal 'what' too, the what/how in the body should offer context and explanation as required.)

by OJFord 3y ago

pdpi 3y ago

I find myself commenting code in a similar pattern: A small kernel of "interesting" code that has a 1:1 ratio (or higher) of comments to code, which enables the rest of the codebase to be "boring" self-documenting boilerplate-y code that doesn’t really warrant much in the way of commenting.

keybored 3y ago

For a commit that adds a language binding (and might be 100+ additions/deletions) I might just say “Add X function”. Because I’m just following established patterns. But for the linked kind of change? Yeah, several paragraphs of explanation is definitely useful.

unregistereddev 3y ago

I agree with you that the little innocuous things often need a longer explanation, but the linked commit message is way too long IMO. It either wastes the readers' time, or it causes the readers' eyes to gloss over at the wall of text. You don't need to document your entire journey in order to document your findings and explain why. > This was a non-ascii whitespace character that caused `ArgumentError: invalid byte sequence in US-ASCII` when running `bundle exec rake` ^ should be sufficient. It includes enough keywords to come up in a search if someone has a similar problem in the future, it contains the root cause of the problem, and it is short enough that people are unlikely to gloss over it.

mbork_pl 3y ago
The article explains why all the rest is, maybe not needed, but good to have.
OJFord 3y ago
Yes, it's not my preferred style either, but it's much better than 'fixes error' type thing, subject line only, that's so common. I like the form: Fix ArgumentError 'invalid byte sequence' Non-ASCII whitespace characters cause [...]. This was apparent in [...] because [...]. This commit fixes the issue by removing the offending character; so the file is now solely ASCII characters. Or that sort of thing. Subject tells me why, body tells me what the problem was and how it was fixed. (Who, when, where are already in the commit metadata! The diff shows a very literal 'what' too, the what/how in the body should offer context and explanation as required.)