top | item 46313614

(no title)

robgibbons | 2 months ago

For what it's worth, writing good PRs applies in more cases than just AI generated contributions. In my PR descriptions, I usually start by describing how things currently work, then a summary of what needs to change, and why. Then I go on to describe what exactly is changing with the PR. This high level summary serves to educate the reviewer, and acts as a historical record in the git log for the benefit of those who come after you.

From there, I include explicit steps for how to test, including manual testing, and unit test/E2E test commands. If it's something visual, I try to include at least a screenshot, or sometimes even a brief screen capture demonstrating the feature.

Really go out of your way to make the reviewer's life easier. One benefit of doing all of this is that in most cases, the reviewer won't need to reach out to ask simple questions. This also helps to enable more asynchronous workflows, or distributed teams in different time zones.

discuss

order

Hovertruck|2 months ago

Also, take a moment to review your own change before asking someone else to. You can save them the trouble of finding your typos or that test logging that you meant to remove before pushing.

To be fair, copilot review is actually alright at catching these sorts of things. It remains a nice courtesy to extend to your reviewer.

Waterluvian|2 months ago

The Draft feature is amazing for this.

I’ll put up a draft early and use it as a place to write and refine the PR details as I wrap up, make adjustments, add a few more tests, etc.

phito|2 months ago

I often write PR descriptions, in which I write a short explanation and try to anticipate some comments I might get. Well, every time I do, I will still get those exact comments because nobody bothers reading the description.

Not to say you shouldn't write descriptions, I will keep doing it because it's my job. But a lot of people just don't care enough or are too distracted to read them.

simonw|2 months ago

For many of my PR and issue comments the intended audience is myself. I find them useful even a few days later, and they become invaluable months or years later when I'm trying to understand why the code is how it is.

ffsm8|2 months ago

After I accepted that, I then tried to preempt the comment by just commenting myself on the function/class etc that I thought might need some elaboration...

Well, I'm sure you can guess what happened after that - within the same file even

walthamstow|2 months ago

At my place nobody reads my descriptions because nobody writes them so they assume there isn't one!

nothrabannosir|2 months ago

This is a hill I’m going to die on, but I find 9/10 times people use the pr description for what should have been comments. “Git blame” and following a link to a pr is inferior ux to source code comments.

The North Star of pr review is zero comment approvals. Comments should not be answered in line, but by pushing updates to the code. The next reader otherwise will have the exact same question and they won’t have the answer there.

The exception being comments which only make sense for the sod itself but not the new state of the code. IME that’s ~10%.

I have bought my tombstone.

skydhash|2 months ago

I just point people to the description. no need to type things twice.

necovek|2 months ago

What is the overall practice in the team? Does everybody write good descriptions and nobody reads them, or only a few of them write good descriptions and nobody reads them?

Because if it's the latter, there's your problem: even those who write good descriptions do not expect a change request to have one, so they don't bother looking.

JohnBooty|2 months ago

Yeah, I've definitely found that nobody reads more than maybe 10 words of the PR description.

I've also never seen anybody but myself write substantial PR descriptions at my previous 4-5 jobs

simonw|2 months ago

100%. There's no difference at all in my mind between an AI-assisted PR and a regular PR: in both cases they should include proof that the change works and that the author has put the work in to test it.

oceanplexian|2 months ago

At the last company I worked at (Large popular tech company) it took an act of the CTO to get engineers to simply attach a JIRA Ticket to the PR they were working on so we could track it for tax purposes.

The Devs went in kicking and screaming. As an SRE it seemed like for SDEs, writing a description of the change, explaining the problem the code is solving, testing methodology, etc is harder than actually coding. Ironically AI is proving that this theory was right all along.

babarock|2 months ago

You're not wrong, however the issue is that it's not always easy to detect if a PR includes proof that the change works. It requires that the reviewer interrupts what they're doing, switch context completely and look at the PR.

If you consider that reviewer bandwidth is very limited in most projects AND that the volume of low-effort-AI-assisted PR has grown incredibly over the past year, now we have a spam problem.

Some of my engineers refuse to review a patch if they detect that it's AI-assisted. They're wrong, but I understand their pain.

reactordev|2 months ago

I do this too with our PR templates. They have the ticket/issue/story number, the description of the ask (you can copy pasta from ticket). Current state of affairs. Proposed changes. Post state of affairs. Mood gif.

brooke2k|2 months ago

I used to be much more descriptive along these lines with my PRs, but what I realized is that nobody reads the descriptions, and then drops questions that are directly answered in the description anyways.

I've found that this gets worse the longer the description is, and that a couple bullet points of the most important things gets the information across much better.

bob1029|2 months ago

> I try to include at least a screenshot

This is ~mandatory for me. Even if what I am working on is non-visual. I will take a screenshot of a new type in my IDE and put a red box around it. This conveys the focus of my attention and other important aspects of the work effort.

mh-|2 months ago

Please just use text for that. PR descriptions on GitHub sufficiently support formatting.

MathMonkeyMan|2 months ago

The amount of work that you put into this comment far exceeds what I typically see in a pull request.

toomuchtodo|2 months ago

This is how PRs should be, but rarely are (in my experience as a reviewer, ymmv, n=1). Keep on keepin' on.