| ▲ | robgibbons 4 days 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. |
|
| ▲ | Hovertruck 4 days ago | parent | next [-] |
| 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 4 days ago | parent [-] | | 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 4 days ago | parent | prev | next [-] |
| 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 4 days ago | parent | next [-] | | 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 4 days ago | parent | prev | next [-] | | 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 4 days ago | parent | prev | next [-] | | At my place nobody reads my descriptions because nobody writes them so they assume there isn't one! | | | |
| ▲ | nothrabannosir 3 days ago | parent | prev | next [-] | | 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. | | |
| ▲ | necovek 3 days ago | parent | next [-] | | But there are really two types of things you need to describe in a change request: - What and why needs changing - What the code does after the change One should really try hard to keep the first one answered in a change request description, or comments in the tool for code reviews. Don't you love running into comments in the code of the type "// This performs better than sorting-after-load as the service offers built-in sorting." because someone originally did "load(); in_memory_sort()" and today the code only does a "load(order_by=X)" (I mean, duh). The resulting code should only have comments that explain the why for the end-state code. But yes, questions to explain something in the end-state should always trigger changes in the code: make code more self-explanatory! | | |
| ▲ | nothrabannosir 3 days ago | parent [-] | | Yes totally agree, I now see a crucial auto correct typo in my original comment wherei was trying to say the same but failed xD. “sod” = “diff”. 100% agree with your comment. |
| |
| ▲ | kubanczyk 3 days ago | parent | prev [-] | | > Comments should not be answered in line, but by pushing updates to the code. Hear, hear. me: This unreadable, needs a comment. them: <explains the thing> me: True, but I've meant a source code comment. them: <explains the thing again, but with more words; push never happens> |
| |
| ▲ | skydhash 4 days ago | parent | prev | next [-] | | I just point people to the description. no need to type things twice. | | |
| ▲ | lanstin 4 days ago | parent | next [-] | | Sadly, when communicating with people, important things have to be repeated over and over. Maybe less so with highly trained and experienced people on something that their training and experience make the statement plausible, but if the thing is at all surprising or diverges from common experience, I've found a need to bang it out via multiple communication channels. | | |
| ▲ | simonw 4 days ago | parent [-] | | I learned this lesson as an engineering manager / tech lead. I got frustrated at how often I found myself having the exact same conversation with different people... until I relapsed that communicating the same core information to different people was a big chunk of the job! |
| |
| ▲ | wiml 4 days ago | parent | prev [-] | | "I think I covered that in the (PR text | comment two lines up | commit message), did you have an issue I didn't address there?" Maybe that's the AI agent I would actually use, auto-fill those responses... |
| |
| ▲ | necovek 3 days ago | parent | prev | next [-] | | 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 3 days ago | parent | prev [-] | | 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 | | |
| ▲ | necovek 3 days ago | parent [-] | | But if nobody writes them, they don't have the habit of reading them either. However, also make sure your PR descriptions are not "substantial" in the "there is a lot of it" sense, but only "substantial" in the "everything of substance is described, but not more" sense :) |
|
|
|
| ▲ | simonw 4 days ago | parent | prev | next [-] |
| 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 4 days ago | parent | next [-] | | 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. | | |
| ▲ | sodapopcan 4 days ago | parent | next [-] | | Complaining about including a ticket number in the commit is a new one for me. Good grief. | | |
| ▲ | rootusrootus 4 days ago | parent [-] | | It could be a death-by-a-thousand-cuts situation and we don't have enough context. My company has spent the last few years really going 1000% on the capitalization of software expenses, and now we have to include a whole slew of unrelated attributes in every last Jira ticket. Then the "engineering team" (there is only one of these, somehow, in a 5K employee company) decrees all sorts of requirements about how we test our software and document it, again using custom Jira attributes to enforce. Developers get a little pissy about being messed with by MBAs and non-engineer "engineers" trying to tell them how to do their job. (as an aside, for anybody who is on the giving end of such requirements, I have to tell you that people working the tickets will happily lie on all of that stuff just to get past it as quickly as possible, so I hope you're not relying on it for accuracy) But putting the ticket number in the commit ... that's basically automatic, I don't know why it should be that big a concern. The branch itself gets created with the ticket number and everything follows from that, there's no extra effort. | | |
| ▲ | comfydragon 4 days ago | parent | next [-] | | > The branch itself gets created with the ticket number and everything follows from that, there's no extra effort. Only problem there is the potential for a deeply-ingrained assumption that the Jira key being in the branch name is sufficient for the traceability between the Jira issue and commits to always exist. I've had to remind many people I work with that branch names are not forever, but commit messages are. Hasn't quite succeeded in getting everyone to throw a Jira ID in somewhere in the changeset, but I try... | | |
| ▲ | necovek 3 days ago | parent [-] | | It sounds pretty simple to automate that away too: make it part of the merge hook to include the source branch name into the message. We are engineers, everything is a problem waiting to be automated :) |
| |
| ▲ | cesarb 4 days ago | parent | prev | next [-] | | > But putting the ticket number in the commit ... that's basically automatic, I don't know why it should be that big a concern. The branch itself gets created with the ticket number and everything follows from that, there's no extra effort. That poster said "attach a JIRA Ticket to the PR", so in their case, it's not that automatic. | | |
| ▲ | rootusrootus 4 days ago | parent | next [-] | | A lot of Jira shops use the rest of the stack, so it becomes automatic. The branch is named automatically when created from a link on the Jira task. Every time you push it gives you a URL for opening the PR if you want, and everything ends up pre-filled. All of the Atlassian tools recognize the format of a task ID and hyperlink automatically. I haven't dealt with non-Atlassian tools in a while but I assume this is pretty much bog standard for any enterprise setup. | |
| ▲ | alexpotato 4 days ago | parent | prev [-] | | If you are using the Atlassian Git clone then just putting the JIRA ticket in the title automagically links the PR to the ticket. |
| |
| ▲ | sodapopcan 4 days ago | parent | prev [-] | | Ah ya, death-by-a-thousand-cuts is certainly a charitable take! |
|
| |
| ▲ | necovek 3 days ago | parent | prev | next [-] | | Invite engineers to solve it in a way that makes it cheap for them. Most shops I've been at prefix their branch names with ticket numbers ("bug-X-" or "TCKT-Y-"), and then it's trivial to reference it back. Some will write scripts on top, which gets them even more motivated to solve your problem (and might add links into the tracking tools too, move the ticket to "In Review" when the PR is up, close it after it's merged...). | |
| ▲ | p2detar 4 days ago | parent | prev | next [-] | | Strange, I thought this is actually the norm. Our PRs are almost always tagged with a corresponding Jira ticket. I think this is more helpful to developers than to other roles, because it allows them to have history of what has been fixed. One can also point QA or consultants to a ticket for documentation purposes or timeline details. | |
| ▲ | 4 days ago | parent | prev [-] | | [deleted] |
| |
| ▲ | babarock 4 days ago | parent | prev [-] | | 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. | | |
| ▲ | wiml 4 days ago | parent [-] | | I don't think we're talking about merely "AI-assisted" PRs here. We're talking about PRs where the submitter has not read the code, doesn't understand it, and can't be bothered to describe what they did and why. As a reviewer with limited bandwidth, I really don't see why I should spend any effort on those. | | |
| ▲ | atomicnumber3 4 days ago | parent [-] | | "We're talking about PRs where the submitter has not read the code, doesn't understand it, and can't be bothered to describe what they did and why." IME, "AI" PRs are categorically that kind of PR. I find, and others around me in my org have agreed, that if you actually do all that you describe, the actual net time savings of AI are often (for a mid-level dev or above) either net 0 or negative. I personally have used the phrase "baptized the AI out of it" describing my own PRs... Where I may have initially used AI to generate a bunch of the code, looked at it and went "huh neat that actually looks pretty right, this is almost done." Then I generate unit tests. Then I fix the unit tests to not be shit. Then i find bugs in the AI-generated code. Then upon pondering the code a bit, or maybe while fixing the bugs, I find the abstractions it created are clunky, so I refactor it a bit... and by the time I'm done there's not a lot of AI left in the PR, it's all me. |
|
|
|
|
| ▲ | reactordev 4 days ago | parent | prev | next [-] |
| 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 4 days ago | parent | prev | next [-] |
| 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 4 days ago | parent | prev | next [-] |
| > 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- 4 days ago | parent [-] | | Please just use text for that. PR descriptions on GitHub sufficiently support formatting. | | |
| ▲ | simonw 4 days ago | parent [-] | | Text isn't good for things like "tighten up the spacing in this dialog". | | |
| ▲ | mh- 3 days ago | parent [-] | | Of course not. Perhaps you misread the comment I was replying to? Quoting from it: > I will take a screenshot of a new type in my IDE and put a red box around it. |
|
|
|
|
| ▲ | 4 days ago | parent | prev | next [-] |
| [deleted] |
|
| ▲ | MathMonkeyMan 3 days ago | parent | prev | next [-] |
| The amount of work that you put into this comment far exceeds what I typically see in a pull request. |
|
| ▲ | toomuchtodo 4 days ago | parent | prev | next [-] |
| This is how PRs should be, but rarely are (in my experience as a reviewer, ymmv, n=1). Keep on keepin' on. |
|
| ▲ | Swannie 4 days ago | parent | prev [-] |
| If only there were community standards for this... Oh, there are, for years :D This has really stood the test of time: https://rfc.zeromq.org/spec/42/#24-development-process And its rationale is well explained too: https://hintjens.gitbooks.io/social-architecture/content/cha... Saddened by realizing that Pieter would have had amazing things to say about AI. |
