101–110 of 431 posts
I guess I was worried that nobody would want to read the commits, but I really like the thought that what I've been providing is a simple narrative thread to help guide a reader through my train of thought.
Earlier quoted context omitted.
For any PR above a few line change, if a developer has not done a self review, I don’t review it all. Instead I request that it is self reviewed with context added, prior to requesting re-review. I also tend to ask the question, “are any of these insights worth adding as comments directly to the code?” 9/10 the context they wrote down should be well thought out comments in the code itself. People are incredibly lazy…
By self review, you mean that the developer adds comments in the code review tool? that is a great idea, I want to try this.
A nice side effect is that going through a self review and adding comments to the PR has helped me catch innumerable things that my coworkers never had to call me on.
Earlier quoted context omitted.
For any PR above a few line change, if a developer has not done a self review, I don’t review it all. Instead I request that it is self reviewed with context added, prior to requesting re-review. I also tend to ask the question, “are any of these insights worth adding as comments directly to the code?” 9/10 the context they wrote down should be well thought out comments in the code itself. People are incredibly lazy…
By self review, you mean that the developer adds comments in the code review tool? that is a great idea, I want to try this.
It's rubber ducking.
Reviewing someone else's large pull request is like having a second task in parallel with what you are working on yourself!
So don't do it in parallel. Completely park other tasks, spend time on the review and record that time appropriately. There's nothing wrong with saying you spent the previous day doing a large review. It's a part of the job, it is "what you're working on".
1) Each PR (even if it's part of a larger whole) can be committed and released independently. 2) Each PR has a description of what it's doing, why it's doing what it's doing, and if it's part of a larger whole, how it fits into the broader narrative. 3) PRs are focused - meaning that QOL or minor bugfixes should not be part of a change that is tackling something else. 4) PRs are as small as possible to cover the issue at hand. 5) All PRs are tested and testing evidence is presented. 6) When a PR is committed to master, the final narrative in step 1) is the Git commit, along with the testing evidence, includes the JIRA ticket number in the headline, and the reviewer in the body of the git commit.
This way we have a clean, auditable, searchable history with meaningful commit history that can help reconstruct the narrative of a change and be used as a guide when looking at a change in, say, a year.
Earlier quoted context omitted.
This is a huge pet peeve of mine. At work I'm an expert on part of the code base that sees a fair number of contributions.I get many private IMs from colleagues asking me to "Approve please" or something like that with a dump of 100s of lines, of which maybe 10 lines are relevant to me (touch files or behaviour that I'm an expert on, hence why they need my approval.) Minimally, I would like context for the change, wh…
> IMO many software developers are just not fast enough at writing or language I think this is the overwhelming factor, software engineering doesn't select for communication skills (and plenty of SEs will mock that as a field of study), or at least most SEs don't start out with them.
Who are these people? I've never encountered that. In my experience engineers who aren't great at communication freely own up to it.
Earlier quoted context omitted.
For any PR above a few line change, if a developer has not done a self review, I don’t review it all. Instead I request that it is self reviewed with context added, prior to requesting re-review. I also tend to ask the question, “are any of these insights worth adding as comments directly to the code?” 9/10 the context they wrote down should be well thought out comments in the code itself. People are incredibly lazy…
By self review, you mean that the developer adds comments in the code review tool? that is a great idea, I want to try this.
- 90% of the time when you self-review your own PR, you're going to spot a bug or some incorrect assumption you made along the way. Or you'll see an opportunity to clean things up / make it better.
- Self-reviewing and annotating your reasons/thought process gives much more context to the actual reviewer, who likely only has a surface level understanding of what you're trying to do.
- It also signals to your team that you've taken the time to check your assumptions and verify you're solving the problem you say you are in the PR description.
The PR type approach comes from the Linux kernel where there is essentially a hierarchy of gatekeepers with increasing trust and responsibility. It is a very individualistic type approach (i.e. if you merged it into your branch, it's on you and speed is a non-goal for the most part). This is often different from many software projects as it is like a collective where often, no one really has individual responsibility…
Earlier quoted context omitted.
> and to ignore my PRs when I don't PRs should be optional, IMHO. Not all changes require peer review, and if we trust our colleagues then we should allow them to merge their branch without wasting time with performative PRs.
Trust, but verify. We're only human after all :-) At $DAY_JOB we need approvals from peers due to industry regulation.