Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

The Theatre of Pull Requests and Code Review

meks.quest

Post reply on HN
Print view View original

241–250 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#241

Post by Too »

crazygringo wrote: post #174

I can't agree with a lot of this. PR's should generally be the size of a feature, or a meaningful subfeature for large features. When you arbitrarily split up PR's into something "300 lines" or "5-10 minutes" you can miss the forest for the trees. The little thing looks fine in isolation but doesn't make any sense as part of a larger approach. Different people are reviewing it piecemeal but nobody is reviewing the ap…

Yeah. While this narrative style tries to explain what things are done, it instead often leaves the question: Why are we doing this at all?

Commit #1 adds a helper function for whatever, looks innocent enough, implementation is correct. Believe it or not, it even has tests, lgtm. Then only by commit #8 do you realize this helper function is not needed at all and the entire approach is wrong. Happens every time.

I started reviewing these chains backwards and refuse starting a review until the whole chain is available. That’s however not always easy either, when commit #2-#5 has incrementally refactored everything into something unrecognizable, so that both the left and right side of the diff are wrong! No, I’m not interested in ”this will be fixed 2 commits down the chain”. I just want to review the final state that goes into production, nothing else matters.

Yes, commits should be made small whenever possible and not include unrelated fixes or refactors. Just please, keep them meaningful on their own.

Re: The Theatre of Pull Requests and Code Review

#242

Post by osigurdson »

stillworks wrote: post #115
osigurdson wrote: post #47

Earlier quoted context omitted.

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.

The interesting phenomenon is the discovery of gamification of such metrics. 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".

It just becomes a huge cycle which is easier for everyone involved than doing actual work that benefits your customers.

Re: The Theatre of Pull Requests and Code Review

#243

Post by muxator »

herval wrote: post #150

I find the sort of opinions on this post quite common on a subset of engineers - namely mid levels with some time in the career, who start to consider themselves senior engineers and want everyone to follow the same set of strict rules they decided make sense. It’s the same mindset that makes people pedantically apply DRY to every situation or forcing others to TDD basic apps. In practice: - smaller PRs aren’t necess…

> - nobody reads intermediate commit messages one by one on a PR, period [...] > > - “every commit must compile” - again, unnecessary overzealousness. [...]

In my part of the world both of these are true, and proudly so. We keep catching a myriad of errors, big and small. The history is easy to read, and helps anyone catching up with how a certain project evolved.

I understand it might not be true for everyone, every team, in every line of business; but this sort of discipline pays off in quality oboth of the code _and_ the team members' abilities.

Re: The Theatre of Pull Requests and Code Review

#244

Post by g-b-r »

osigurdson wrote: post #237
g-b-r wrote: post #108

Earlier quoted context omitted.

If security or bugs don't matter much for your project, sure

I don't think "LGTM" helps with either of those. Making someone personally responsible is the real unlock here.

Personally responsible for the review?

For sure if you can say LGTM without even looking at anything it doesn't make much sense

Re: The Theatre of Pull Requests and Code Review

#245

Post by bob1029 »

roguecoder wrote: post #170
bob1029 wrote: post #81

I see the primary value of a pull request being the simple awareness of what is being worked on. I aggressively sync my local changes to PRs that are marked draft in GitHub. Other developers I work with do the same. Throughout the day we asynchronously check in on the scope of the others' work. If there is something that looks like it might conflict, we call a meeting. The actual code review phase for me is more abou…

Why do you see feedback as "purity test"ing? Newspaper reporters are professionals and they still have editors. We've all seen the disaster that results when an author gets too famous to edit. And we don't have to go in and work with their prose later. Code reviews are where "my" code becomes "our" code: I want my coworkers to feel comfortable with and fully understand and be happy to support the changes I am proposi…

> Why do you see feedback as "purity test"ing?

I didn't say this.

There are many forms of extremely valuable feedback that do not involve subjecting your teammates to a ritual of "clean code" every time a PR is submitted.

Re: The Theatre of Pull Requests and Code Review

#246

Post by OsrsNeedsf2P »

roguecoder wrote: post #181
conartist6 wrote: post #29

If your goal is to lower the velocity of your organization, e.g. because in practice code churn or poor quality are major problems, then by all means do this. If you still need to move fast, then don't. This is the "don't run in the hallways" version of software culture, but I would contend that you should choose your pace based on your situation. It's just like gradient descent really. The be efficient sometimes you…

Changing cowboy code is much, much slower than changing good code. If you need to move fast for the next two weeks, sure. If you need to move fast for the next year, you are better off collaborating.

You're making the faulty assumption the whole project won't get scrapped

Re: The Theatre of Pull Requests and Code Review

#247

Post by atoav »

jvanderbot wrote: post #5

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…

Exactly, if you want people to think about your code/changes you should be able to give them the needed context.

If you don't know them, please realize your code isn't automatically a gift everybody waited for, you may see it that way, but from the other side this isn't clear until someone put in the work to figure out what you did.

In short: added code produces work. So the least you should do is try reducing that work by making it easy to figure out what your code is and isn't.

Sum up what changes you made (functionally), why you made them, which choices you made (if any) and why and what the state of the PR code is in your own opinion. Maybe a reasoning why it is needed, what future maintenance may look like (ideally low). In essence, ask yourself what you'd like to know if someone appeared at the door and gave you a thumb drive with a patch for your project and add that knowledge.

Also consider to add a draft PR for bigger features early on. This way you can avoid programming things that nobody wanted, or someone else was already working on. You also give maintainers a way to steer the direction and/or decline before you put in the work.

Re: The Theatre of Pull Requests and Code Review

#248

Post by mtoner23 »

danielfalbo wrote: post #78

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 -…

This is also the system we used with gerrit. Googles PR system. It uses cherry picks instead of branches but it's fantastic and gracefully splits the PR work between reviewer and committer

Re: The Theatre of Pull Requests and Code Review

#249

Post by horsawlarway »

1dom wrote: post #192
MoreQARespect wrote: post #180

Earlier quoted context omitted.

Give me one example then. One is all it takes to disprove a rule. Im fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example? The only exception i can think of is something where 99.9% of the changes are autogenerated (where i wouldnt really be reading it carefully anyway, so the length is immaterial...).

I agreed with you initially. > I'm fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example? To me, when I meet experts in any field, the quality that stands out isn't that they do everything to expert level, it's that they get everything done as they said they would. Sometimes that means big PRs, because that's the environment creat…

In my experience - this one is the correct one. Make a commitment, keep the commitment, stay responsible for the commitment afterwards.

This whole chain is like arguing on how tidy your desk should be. Some people like it fastidious to the nth degree. Some people prefer a little mess.

In neither case does that preference really matter much compared to all the other things a real job entails.

Re: The Theatre of Pull Requests and Code Review

#250

Post by sodapopcan »

jonahx wrote: post #200
herval wrote: post #150

I find the sort of opinions on this post quite common on a subset of engineers - namely mid levels with some time in the career, who start to consider themselves senior engineers and want everyone to follow the same set of strict rules they decided make sense. It’s the same mindset that makes people pedantically apply DRY to every situation or forcing others to TDD basic apps. In practice: - smaller PRs aren’t necess…

> nobody reads intermediate commit messages one by one on a PR, period. I worked on a team where the lead was adamant about this and started to write messages in the vein of “if you’re reading this message, I’ll give u $5”. I never paid anyone a dollar. Don’t waste your time writing stuff for no one. I do. Especially if the author is competent. That said, empirically, you're correct most people don't. However, that s…

Also replying to echo this. Hate these blanket statements.
Post reply on HN