111–120 of 431 posts
Every developer I know who applies this sort of “highly documented development” approach where they “work through their thought process openly.” Is only doing it because their thought processes are already so funky and counterintuitive that reviewers actively reject their work unless have written evidence that the developer didn’t just entirely change the scope of their assignment to justify the bizarre decisions.
Jane Street implements an awesome code review system: https://janestreet.com/tech-talks/janestreet-code-review > [...] Telling a Story with Commits [...] > [...] it should take the average reviewer 5-10 minutes [...] Jane Street code review system kinda solves this problem by - making each commit a branch, - stacking branches on top of each other (gracefully handling rebases and everything that comes with it), and -…
PR review is probably at least a little performative. But I trust my colleagues to do good reviews when I ask them to, and to ignore my PRs when I don't. That's kind of the way we all want it. I regularly ask for a review of specific changes by tagging them in a comment on the lines in question, with a description of the implications and a direct question that they can answer. This, "throw the code at the wall for in…
> 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.
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…
> Minimally, I would like context for the change, … … the why is important > IMO many software developers … don't have explanations to provide. People not taking the time is what makes reviews performative. … a lot of developers only consider the how . i’ve had a lot of experiences of “once my PR is submitted that’s my work/ticket finished” kind of attitude. i spent a year mentoring some people in a gaming community…
The why is someone else's job, so the developers should just ask them for a blurb to put in the PR for context, along with a note to the reviewer to ask that person for even more context if necessary.
Earlier quoted context omitted.
In some parts of the industry number of CRs and revisions-per-cr is tracked as a performance metric. Many people learn to game this to make their "numbers" appear good i.e. high number of CRs and low revisions per CR.
Easy and fun to put metrics around random things. I'll start to squirm if you ask me to draw the connection between these metrics and NPV.
I do see the value in breaking down larger chunks of work into logically smaller units of work and then produce multiple pull requests where needed.
But some people are really clever and influential and manage to game these numbers into "apparent success".
i don't know how people can build something and leave behind a coherent series of commits that tell a nice story of progressively building a thing. my commits include lots of false-starts that get abandoned and "i need to commit this interim state because i deprioritized this and will come back later". the sequence of events that i used to build the thing isn't necessarily the best sequence of events to tell the stor…
Let main be the story of how code got from point A to B, and PRs be the story of how each incremental step was made.
Also find doing it like this either incredibly hard or have to do a ton of git magic after I'm done to get commits into this state which is very frustrating.
I think it might be the codebase I work on but who knows.
It is very telling in the article itself there is a screenshot of the commits tab in the PR workflow that many don't realize even exists and/or never think to use.
In the Files tab the commit picker has gotten better in recent years, but it is still overly focused on selecting ranges of commits over individual ones, and there's no shortcuts to easily jump Next or Previous commit, you have to remember your place and interact with the full pulldown every time. Also, it's hard to read the full descriptions of commits in the Files view and I find I often have to interrupt my flow to open the commit in another browser tab or flip back and forth between the Commits tab and the Files tab in the PR. The Commits tab also defaults to hiding most of the commit descriptions, so it's still not a particularly great reading experience.
It feels like a bit of a bad feedback loop that GitHub's UI doesn't make commit-by-commit reviewing clean/easy because GitHub themselves don't expect most developers to write good commits, but a lot of developers don't write good commits today simply because GitHub's PR interface is bad at reviewing individual commits and developers don't see as much of a point in it if they aren't going to be reviewed in that way.
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.
You could argue this is what commits are for, but given how people use GitHub and PRs, it gives some extra visibility.
And if you're going to use AI to assist you when writing the code I would argue this self-review step is 100% mandatory.
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.