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

311–320 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#311

Post by g-b-r »

kiitos wrote: post #296
g-b-r wrote: post #216

Earlier quoted context omitted.

> Intermediate commits can have meaningful commit messages if the author chooses, but they could also just be labeled "wip" and still be useful to the author. > It's really easy for a note someone writes to themselves to be useful to that person without being useful to other people. After a few months it will probably be as useful to you as to anyone else; if you only use commits as some sort of help while developing…

> if you only use commits as some sort of help while developing, you might as well just squash them before making a PR. yeah for sure you want to squash-merge every PR to main, right? commits are just commits, there is no moral value to them, there is no "good history" or "bad history" of them, whether or not they're "made thoughtfully" isn't really interesting or relevant git is just a tool, and commits are just a m…

> yeah for sure you want to squash-merge every PR to main, right?

Oh god you're serious?

> git is just a tool, and commits are just a means to an end

To more ends than you realize, probably, if you put some care in making them

Re: The Theatre of Pull Requests and Code Review

#312

Post by MoreQARespect »

overfeed wrote: post #203
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...).

> 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? Not couldn't - but shouldn't , such as when there's tight coupling across many files/modules. As an example, changing the css classes and rules affecting 20+ components to follow updated branding should be in one big PR[1] for most branching strategies. Sometimes it's eas…

>Not couldn't - but shouldn't, such as when there's tight coupling across many files/modules.

No, this is a pretty classic example of where you can break up the work by first refactoring out the tightly wound coupling in one PR before making the actual (now simpler/smaller) change in a second PR.

Re: The Theatre of Pull Requests and Code Review

#313

Post by sfn42 »

singpolyma3 wrote: post #271
ownagefool wrote: post #263

Earlier quoted context omitted.

Yeah, I agree with both you and the GP. There's a mess of commits that usually don't matter because mostly only the before and after level of an actual viable PR does, ergo I squash them. I'm cool with other reasonble approaches though, but I'm pretty over pointless hoops because someone says so.

If the commits don't matter why did you make them separate to begin with?

I'm not the person you asked, but I often don't. I personally don't care about git history - I read code not commit messages. I don't really care how it got the way it is I care about what it is, maybe once in a blue moon there could potentially be some useful information in a commit message but it's not enough.

So, I'll often just make lots of changes and then commit them all at once with some vague commit message and that's that. Nobody cares. If I want to tell people why the code is the way it is I'll just add a comment.

This is how I work. I have tried to be more disciplined with commits and stuff like that but I find that it just slows me down and makes the work feel more difficult. I also frequently just forget and find myself having made lots of changes without any commits so then I have to retroactively split it up into commits which can be difficult too. So I'd rather just not worry about it, focus on getting good work done and move on rather than obsess over a git history that's unlikely to ever be read by anyone. I realize that's a self-fulfilling prophecy in that it'd be more likely to be read if it was useful and well done but it's not just me. If I was in a team where everyone did it really well I'd try to keep my own work up to par. But usually I'm the one who cares most about how we do things and this just doesn't seem important to me.

Re: The Theatre of Pull Requests and Code Review

#314

Post by IshKebab »

keybored wrote: post #309
IshKebab wrote: post #222

Earlier quoted context omitted.

IMO there's no point having a clean history of commits within a PR. With rare exceptions, if you have a PR with a clean history of commits and each commit compiles and passes the tests... they should be separate PRs! If it isn't clean then it should be squashed. A few exceptions: 1. When refactoring often your PR is "do an enormous search and replace, and then fix some stuff manually". In that case it's way easier to…

> IMO there's no point having a clean history of commits within a PR. With rare exceptions, if you have a PR with a clean history of commits and each commit compiles and passes the tests... they should be separate PRs! If it isn't clean then it should be squashed. A perfect illustration of a backwards mindset. If this made sense then the standard or least common denominator PR tool would work better with many small P…

Yeah it's kind of infuriating really. It's not like it's an uncommon workflow either. Everywhere I've worked people end up with PRs that just say "this depends on this other PR; ignore the first commit".

Gitlab kind of supports it - if your second PR's target branch is the first PR then it will only show you the code from the second PR and it will automatically update the target branch to master when the first one gets merged. I wouldn't say it's first class support though.

Sapling sort of has support for making it work on GitHub: https://sapling-scm.com/docs/addons/reviewstack/

And there was some forge that supports Jujutsu that has proper first class support, but I can't find it now.

Anyway it's a very useful workflow that lots of people want and kind of insane that it isn't well supported by GitHub.

To be fair I can't remember the last time GitHub introduced any really new features. It's basically in maintenance mode.

Re: The Theatre of Pull Requests and Code Review

#315

Post by kiitos »

g-b-r wrote: post #311
kiitos wrote: post #296

Earlier quoted context omitted.

> if you only use commits as some sort of help while developing, you might as well just squash them before making a PR. yeah for sure you want to squash-merge every PR to main, right? commits are just commits, there is no moral value to them, there is no "good history" or "bad history" of them, whether or not they're "made thoughtfully" isn't really interesting or relevant git is just a tool, and commits are just a m…

> yeah for sure you want to squash-merge every PR to main, right? Oh god you're serious? > git is just a tool, and commits are just a means to an end To more ends than you realize, probably, if you put some care in making them

i just don't use commits like you do, and that doesn't mean i'm being less careful or less thoughtful, or that my changes are worse than yours

commits are what i say they are, nothing more or less

Re: The Theatre of Pull Requests and Code Review

#316

Post by MoreQARespect »

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…

>I'm not doubting you _could_ break up any PR into a shorter one. But that's kind of the point of an expert: they recognise what makes sense to do in reality

I have seen plenty of huge PRs which were more trouble than they were worth to break up after discovery. At some point it becomes like unbaking a cake. It's a trade off.

Ive just never thought when I saw any of them that there wasnt a more practical way to get there with a bunch of smaller PRs.

Unlike dealing with an already existent large PR this isnt really a trade off thing - there are basically almost no circumstances when it is preferable to review one 1000 line code change instead of 4x self contained 200 line changes.

Re: The Theatre of Pull Requests and Code Review

#317

Post by jonahx »

William_BB wrote: post #256
jonahx wrote: post #200

Earlier quoted context omitted.

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

PR is the atomic level of work. I'd argue PR-level history (i.e. squash) is often enough and is way cleaner. Why would I care about "commit A", "change parts of A because I misunderstood a requirement", "improve A based on code review" etc.? If I want that granularity, I'd go read the original PR and the discussion that took place.

> PR is the atomic level of work.

The atomic level of work should be a single, logically coherent change to the codebase. It's not managerial, it's explanatory.

As you work things naturally arise. Over here a reformatted file, over there comments to clarify an old function that confused you, to help the next developer who encounters it. Cleaning, preparatory refactoring that is properly viewed as a separate action, and so on. Each of these is a distinct "operation" on the codebase, and should be reviewed in isolation, as a commit.

Some of these operations have nothing to do with the new feature you're adding. And yet creating separate PRs for each of them would be onerous to your reviewers and spammy. Clean, atomic history lets you work naturally while still telling a clear story about how the code changed, both for reviewers and future developers.

Re: The Theatre of Pull Requests and Code Review

#318

Post by smcameron »

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…

> - “every commit must compile” - again, unnecessary overzealousness.

So you're the one breaking git bisect all the time. Grrrr.

Use stgit and make decent commits instead of rolling in the dirt like an animal.

Re: The Theatre of Pull Requests and Code Review

#319

Post by xmcqdpt2 »

sardines wrote: post #190
xmcqdpt2 wrote: post #44

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…

My team uses a github PR template with the following sections. Answers to each can be short yet it has been extraordinarily helpful to pass over important info to the reviewer that's not captured in code. It also borders on "checklist" that the code author has actually done the bare minimum to think things through. # Goal (why is this change needed at all) # What I changed and why I did it this way # What I'm not doi…

That's a great idea, I should do that. There is another team that does something similar but everyone complains about it (it's not as good as your template).

Re: The Theatre of Pull Requests and Code Review

#320

Post by sfn42 »

nicksergeant wrote: post #107
tcoff91 wrote: post #92

Earlier quoted context omitted.

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.

Yep. Self-reviewing your own PRs is a large boost to both yourself and the team, and often one of the first things I encourage new-ish developers to do. - 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 mu…

I always review my own PR before I expect someone else to, but I generally don't add comments. I just look it over and if I see something I want to fix I fix it. Adding comments for things I specifically want feedback on or am unsure about seems like a nice addition to the process though. I might start doing that too.
Post reply on HN