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

401–410 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#401

Post by [deleted] »

sigseg1v wrote: post #392
illuminator83 wrote: post #321

Earlier quoted context omitted.

I always tell my engineers to create atomic commits and we usually review commit by commit. Obviously commits like "fixed review comments" or "removed some left-over comments" or "fixed typo" should not be pushed into a PR you asked others to review. I expect people to understand how to clean their commit history - if they don't I teach them. The senior people who are capable of structured work - e.g. are used to con…

> Obviously commits like "fixed review comments" or "removed some left-over comments" or "fixed typo" should not be pushed into a PR you asked others to review. Could you explain this a bit more? I'm having trouble visualizing the end to end process. 1. Someone has what they feel is a complete change and submits a PR for review. 2. The reviewers read part of it, first half looks good, and halfway through they have co…

[deleted]

Re: The Theatre of Pull Requests and Code Review

#402

Post by bccdee »

jonahx wrote: post #317
William_BB wrote: post #256

Earlier quoted context omitted.

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

> The atomic level of work should be a single, logically coherent change to the codebase.

Sure, and we must make that correspond to the atomic unit that our collaboration tools provide us for reviewing and merging. In Github and similar git forges, that's a PR, not as a commit. A string of atomic changes should be represented as a series of PRs, not a series of commits in one PR, because Github isn't designed to review and merge individual commits.

The "atomic commits" crowd are (in my opinion) coming up with best practices for the tools they wish they had and working against the grain of the tools we actually use.

Re: The Theatre of Pull Requests and Code Review

#403

Post by mixmastamyk »

imiric wrote: post #375
mixmastamyk wrote: post #363

Earlier quoted context omitted.

Virtue signaling is not a business need. If you want to communicate with others, write proper docs in a format that won't be lost to time, and are accessible to everyone, not merely investigative developers.

Virtue signalling? What are you on about? Everything I said has direct benefits for the team, and hence for the company. > If you want to communicate with others, write proper docs in a format that won't be lost to time You have a severe misunderstanding of what commit messages are for. They're meant to describe changes that can be used as historical reference by developers . They're not meant to be read by non-devel…

> And... you're actually proud of this? Amazing.

Your posts above are dripping in it.

Docs are available to everyone, accessibility in action. You have a severe misunderstanding of what communication is.

There’s no important developer information that should be explicitly and effectively hidden from others. There’s not even a proper search facility, you have to browsing with a lot of background knowledge until you hopefully find something. Newer members won’t have this knowledge.

Code changes, requirements change, often. Info becomes obsolete rather quickly. Projects may last decades. By definition, historical assumptions are inferior. There’s already a mechanical commit record as well.

So yes, buying any important information there is going to be lost to time, and is therefore a waste of it.

Re: The Theatre of Pull Requests and Code Review

#404

Post by mixmastamyk »

imiric wrote: post #377
mixmastamyk wrote: post #362

Earlier quoted context omitted.

We use it to save our progress, backup files, communicate with others. You know, the main benefits of version control? Writing a story no one will ever see is not one of them. Write real docs and your PM, QA, SMEs will benefit as well, not only developers who bother to dig thru the history.

> We use it to save our progress Saving progress is useless if your history is a mess and you have no idea what a previous state contains. > backup files, communicate with others. You do know that there are better tools than a VCS specifically built for these use cases, right? > You know, the main benefits of version control? No, I don't think you understand what version control is for. You can use a knife to open a…

> Saving progress is useless if your history is a mess…

Nope, it works, commit! Tests pass, commit! Push.

You’ve demonstrated you don’t know what version control is for. Cleaning up the past is a peripheral nicety, that is not at all core.

In fact some situations prefer history not be changed at all.

Re: The Theatre of Pull Requests and Code Review

#405

Post by jonahx »

bccdee wrote: post #402
jonahx wrote: post #317

Earlier quoted context omitted.

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

> The atomic level of work should be a single, logically coherent change to the codebase. Sure, and we must make that correspond to the atomic unit that our collaboration tools provide us for reviewing and merging. In Github and similar git forges, that's a PR, not as a commit. A string of atomic changes should be represented as a series of PRs, not a series of commits in one PR, because Github isn't designed to revi…

This is simply not true.

There is a "commits" tab and next button to quickly go through commits on every PR. It's very easy to use.

All that you mean is most people ignore it.

Re: The Theatre of Pull Requests and Code Review

#406

Post by phatskat »

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.

> Why would I care about "commit A", "change parts of A because I misunderstood a requirement", "improve A based on code review" etc.?

For me it’s because Feature A may largely be fine but one of those intermediary commits introduced a regression. I can bisect and isolate an issue much more easily if I have the full history to step through as opposed to “this big commit intrigued a one-line regression _somewhere_ in a 900 line commit”

Re: The Theatre of Pull Requests and Code Review

#407

Post by phatskat »

siva7 wrote: post #302
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…

I've met thousands of developers over my career and i could put them into two categories: those who don't give a shit about intermediate commit messages (majority) and those who browse every single intermediate commit message in a PR (very few). To be honest, the latter had some tendency to be difficult to work with. It was also a useful discriminator to avoid getting those into my teams.

I tend to read intermediate commits because it can be helpful in understanding how the engineer thought through developing the feature. This is especially informative when reviewing more junior/mid-level code, or when a feature grows beyond what I would consider acceptable scope - obviously, avoiding these kinds of branches is the ideal state, and unfortunately the realty doesn’t let me always push back for smaller PRs.

Re: The Theatre of Pull Requests and Code Review

#408

Post by ornornor »

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…

> reads intermediate commit messages

> every commit must compile

I’m in the opposite camp. Following these two practices often doesn’t make any difference but the few times it did saved me a ton of time.

Dropping commits or rebasing is much easier when you have descriptive, atomic commits. It’s also helpful when performing git blame archeology to try and understand why this code looks so weird and has no context. It’s also useful when bisecting (not so much a problem with small PRs, quite handy as they grow bigger and bigger)

As with everything it’s about context and circumstances. As you gain expérience you can appreciate and gauge when it’s required. When you don’t have the expérience then you follow rules so that you gain said expérience. That’s how I see it.

Re: The Theatre of Pull Requests and Code Review

#409

Post by leetrout »

m000 wrote: post #333
leetrout wrote: post #157

Earlier quoted context omitted.

My simple suggestion to my teams: PRs are emails to your team and to your future self. Framed in that context it's easier to carry the correct tone and think about scoping / what's important. --- > pedantically apply DRY to every situation I swear DRY has done more damage to the software industry from the developer side than it has done good because it has manifested into this big stick with which to bludgeon people…

> PRs are emails to your team and to your future self. This should be commits though. Typically, developers would look for clues in this order: code -> code comment -> commit message -> PR text -> external document So commit messages puts the information closer to the user. One hop doesn't seem much, but the time saved adds up as you go. Also, as some other reader mentioned anecdotally, PRs may not be there forever.…

I agree with you. It's how the best commits are on Terraform.

Everywhere I've worked the past few years squashes PRs on merge with the PR becoming the commit title + message so the context lives on in the git history.

Re: The Theatre of Pull Requests and Code Review

#410

Post by bccdee »

jonahx wrote: post #405
bccdee wrote: post #402

Earlier quoted context omitted.

> The atomic level of work should be a single, logically coherent change to the codebase. Sure, and we must make that correspond to the atomic unit that our collaboration tools provide us for reviewing and merging. In Github and similar git forges, that's a PR, not as a commit. A string of atomic changes should be represented as a series of PRs, not a series of commits in one PR, because Github isn't designed to revi…

This is simply not true. There is a "commits" tab and next button to quickly go through commits on every PR. It's very easy to use. All that you mean is most people ignore it.

And then when people put comments on your commits and you force-push new ones, does it link the updated version of each commit to its previous self, giving a clear timeline of comments and changes? I don't think so. But if people write comments at the MR level and you fix them in new commits, then the throughline is clear.

I think a workflow like this for atomic commits would be nice. tangled.sh supports it for jujutsu¹, and it looks really neat. But the existing code review interface is clearly designed for code review to take place at the MR level.

[1]: https://blog.tangled.org/stacking

Post reply on HN