Comments in the code vs PR description
9 points by abareplace
9 points by abareplace
While reading the article I got a weird uncomfortable feeling about putting all this information in the PR. After sitting with that for a while I realized what it was.
This is very valuable information, putting it all in the PR means that it's lost when you want to take your project to a different git server. I'm not surprised that this is not a consideration coming from a microsoft blog post, but as a FOSS developer this is a terrible idea.
My suggestion would be to treat PRs as bureaucracy. Keep the discussion scoped to the process, e.g. "this is what you need to do for this change to be accepted". The information that the author suggests seems to belong, imo, to the ticket. That's "the business side". Then there's the code, and PRs are the bridge between the two.
I don't know, that's how I see it, but to be fair I've worked in all kinds of environments where they worked the way the author describes too. So at the end of the day, if it works for you... :D
it's lost when you want to take your project to a different git server.
it's lost way earlier - once it's merged, finding relevant MR/PR can be PITA and it's not in the top3 places I'd search for it (code, docs, commits)
especially if there was some repo-wide refactoring and it changed places (but it's not as big concern, as code comments also get detached from relevant lines sometimes)
Good point. In fact the best way to find a PR is to go to the ticket and find all the associated PRs (assuming you have that kind of automation; if you don't, you should). Again, PRs are a form you need to fill to get your change merged, they are bureaucracy (and I mean this in a positive way, the right amount of bureaucracy is good).
This is very valuable information, putting it all in the PR means that it's lost when you want to take your project to a different git server
I think you are assuming that the 'PR' message is distinct from the commit message. I don't think that was the expectation here.
I think you are assuming that the 'PR' message is distinct from the commit message.
Why "commit" in singular? What happens with the description if you have more than one? The author makes no mention of commits in their article, I see no evidence to your point.
Why "commit" in singular?
Because that's the atomic unit.
What happens with the description if you have more than one?
Each commit message describes a change. If you need to read more than one commit message to understand what a change is, then that's a problem.
The author makes no mention of commits in their article, I see no evidence to your point.
The distinction he's making is between the messages that live in the code and the messages that describe changes.
I don't see how you would read these as anything other than commit messages. What system are you using where the commit messages are not the messages that you read when reviewing a PR?
In all of my working career, it has been a struggle to convince people to write meaningful messages directly into the commits themselves rather than as the PR description. The companies I've worked at have PR description templates that people fill out, neglecting more granular information in the commit messages themselves.
Some of the articles comments mention this, but not the article itself; commit messages should not be neglected here. This is why I prefer not to squash and merge unless the commit messages are concatenated but, even then, the granularity and ability to bisect is compromised. For me, PR descriptions are for communicating to the reviewers why they should merge your PR as the post suggests, inline comments are for explaining why the code is the way it is (and maybe why not some other way) as well as for anything not obvious about the code it’s attached to, and commit messages (and the commits themselves) are for a durable timeline of what was done and why (which is information that can be lost in a collapsed snapshot of what the code looks like at any given point). One of the things that’s hardest to teach and establish at a company is good commit hygiene where that is lacking because people don’t want to interactive rebase among other headaches, but I will always swear by it
I find PR description to be hostile to developers that want to keep context in the git tree. I understand the squash and merge will preserve the or description but what about during review? As im working on a PR I’m using JJ absorb as much as possible to keep the commits discrete and categorized by the progression of the change.
PRs can also be DMs or emails—& to non-Git VCSs. The commit/patch is a better place for a description for history since it will port when you not only change forges (which is already missing DMs) but also most VCS migration tools will preserve these messages.
There are many kinds of stuff that can go in the PR description that is not suitable for the code or the commit messages. For example, anything related to the review: which kind of review do you want people to do, things to pay attention to, things you already tested yourself, hints about testing, etc.
Also very temporary context (such as, "per yesterday's discussion...", etc.)
In any case, insert perpetual GitHub reviews suck statement. I think they make it harder to review code and to submit atomic changes.