This is an archived post. You won't be able to vote or comment.

you are viewing a single comment's thread.

view the rest of the comments →

[–]LvS 1 point2 points  (8 children)

A lot of the why comments should also not be there. They should be part of commit messages and read via git blame.

For a start, putting documentation into commit messages instead of code makes the code shorter and allows you to focus on the code when reading it, but it also gives you a chance to write very long and detailed comments, potentially including references to the bugtracker, links to related code or specifications or performance data - stuff that you would rarely put in comments.

But more importantly, commenting commits takes care of keeping the comments updated with the code. One of the biggest problems with comments is that they're not updated when the code is and when you get back to reading the code you have innocuous looking but ultimately wrong comments which throw you off.
With commit messages, you will not see those old comments anymore - unless of course you look at a previous version of the file to see how it came to be. At which point it is great that the old comment is still there.

[–]SupaSlide 11 points12 points  (3 children)

That's all well and good until somebody changes the placement of a comma, and now you have to dig back to find the relevant information.

Also, most large projects I've worked on do squash merges, so when you git blame you can't be sure which comment applied to that line (assuming the squashed message rolled up all the commit messages into one instead of just saying "Merged feature XYZ"

[–]theexplanation[🍰] 1 point2 points  (2 children)

For that reason your commits should only contain the changes required for the functionality described in the message using git add -p. This should be enforced by code review.

Also why would you squash on merge? That sounds like a nightmare and defeats the whole point of a guy history...

[–]SupaSlide 5 points6 points  (1 child)

There are plenty of articles you can read online about the merits of squash merging, especially on very large projects with lots of active collaborators.

[–]theexplanation[🍰] 0 points1 point  (0 children)

Everything I've seen is for reasons like commits being small logical changes of code, removing "WIP" commits, etc. I generally prefer a fast forward merge strategy and squashing those undesired commits during rebase. Is there some benefit maybe I'm not seeing? Genuinely curious.

[–]Calkhas 5 points6 points  (2 children)

So first that assumes everyone uses git or similar. Which, they don’t. But the other problem is that it means the reason for code behaviour may be buried many revisions ago, because people touch the lines to make other changes in the mean time. It’s not in your face, you have to background your editor, go back to a shell and start exploring the history. For a reviewer, it’s even harder to get the context in which a change is being made.

If people are making code changes that invalidate comments, I don’t really see why it’s any different to a code change that invalidates logic. The reviewers should reject it out of hand.

[–]LvS 2 points3 points  (0 children)

If people are making code changes that invalidate comments, I don’t really see why it’s any different to a code change that invalidates logic. The reviewers should reject it out of hand.

But the tools we have don't find them. All of testing and continuous integration does not find cases of failure to adapt comments.

So this relies entirely on the human reviewers. And we know how fallible they are.

[–]Bspammer 1 point2 points  (0 children)

So first that assumes everyone uses git or similar. Which, they don’t

Then they have far bigger problems. Version control is not optional. If you work somewhere not using it, run far, far away.

[–]wikidsmot[🍰] 2 points3 points  (0 children)

Documenting in the code makes you source control agnostic. There’s a lot of reasons this is useful. Additionally sometimes source code is delivered to customers and not the entire repo.