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

On Code Review

hugodias.substack.com

Post reply on HN
Print view View original

51–58 of 58 posts

Re: On Code Review

#51

Post by NotAnOtter »

V-2 wrote: post #5

"Approving a pull request without even testing the code it’s very dangerous" Well; code review is not QA. My approval means - I'm OK with how the code layer is knitted. It doesn't mean I've tested the changes. Of course, if there's some logical problem with the implementation (such as the author seemingly failing to handle an edge case), any careful reviewer should catch this out. Still, this is a situation where the…

A submitted code review comes with the following assumptions

1. The submitter approves of the code being merged into master 2. The submitted has tested the happy path and everything seems to work

Re: On Code Review

#52

Post by WorldMaker »

pixel_tracing wrote: post #39
siva7 wrote: post #35

Earlier quoted context omitted.

It’s a matter of efficient role separation. Most teams i’m aware of do not manually test a PR in the same cycle as a code review. It is absolutely expected that the PR author already tested his changes sufficiently. The manual test is usually reserved for the product or QA team.

Good luck getting junior or mid level engineers to do this properly each and every single time :) hell I’ve seen even seniors do this improperly

It can be an automation thing. With tools like (Github) Codespaces there are places today that have the ability to deploy every random PR branches and do PRs in a full code editor backed with a running deployment that you can browse. Admittedly, that's a lot easier to do when what you are deploying is web-based and automating spinning up new URLs (even just proxy forwarded localhost URLs in the case of Codespaces) is easy and things are a lot harder to automate with for instance mobile apps.

Re: On Code Review

#53

Post by WorldMaker »

KronisLV wrote: post #21
V-2 wrote: post #19

Earlier quoted context omitted.

"You don't actually want these changes to end up in the main branch before they are done." True - and it sounds like a textbook scenario for introducing a feature branch... this approach has worked well for me. The idea of queueing up "sub-requests" is interesting, but it comes at the cost of creating another layer of complexity (in my view).

Agreed! I guess what i'm trying to say (in an awfully wordy way) is that we could probably improve the UI for pull/merge requests and gain a lot from it. I mean, GitHub and GitLab already improved many development workflows with their inline comments (as well as other features, like code recommendations) and it feels like sooner or later more improvements are bound to come!

I don't have as much experience with GitLab, but I actually think Github has a lot of tools in their PR system today for doing "early" or "work in progress" PRs incrementally. You can start a PR on an empty branch. You can apply a label "Work in Progress" and start discussion immediately. If you review a chunk of code the PR is pretty good about showing you what you've already reviewed versus what is new in the most recent update. If for some reason you feel a need to start over and force push an entirely new branch to the PR, Github's PR system handles it better than you would expect (and auto-closes old comments, etc).

I've found the tools are better than people expect them to be and the hard part for me is convincing junior developers that it is "okay" to start PRs "early", that they don't have to feel like they've finished a task to get comments/reviews on work in progress. There's a lot of embarrassment/group politics in the way sometimes from using "PR early" workflows.

Re: On Code Review

#54

Post by WorldMaker »

pm215 wrote: post #23
eertami wrote: post #18

Earlier quoted context omitted.

> because I care about the convenience of the person that is actually interested in investigating a feature/bug 5 years from now and not of the lazy code reviewer now Doesn't seem like such a big problem to make a PR with multiple commits, and then squash and merge into 1 commit after the review.

Personally I think that is pessimal, because if I am the person investigating in 5 years time I want to see the change split up into multiple logical commits. So lots-of-commits-plus-squash-and-merge requires the developer to do the work of splitting up the feature implementation but doesn't give the benefit of having a usefully split set of commits to the future-debugging-person...

I agree. If you want the "clean" straight line from the git DAG just use --no-ff in your PR system and you get a nice clean integration log with things like git log --first-parent (git praise --first-parent, git bisect --first-parent) and can still keep the extra history around for those of us that find it useful when investigating things 5/10 years down the road.

Re: On Code Review

#55

Post by boleary-gl »

WorldMaker wrote: post #53
KronisLV wrote: post #21

Earlier quoted context omitted.

Agreed! I guess what i'm trying to say (in an awfully wordy way) is that we could probably improve the UI for pull/merge requests and gain a lot from it. I mean, GitHub and GitLab already improved many development workflows with their inline comments (as well as other features, like code recommendations) and it feels like sooner or later more improvements are bound to come!

I don't have as much experience with GitLab, but I actually think Github has a lot of tools in their PR system today for doing "early" or "work in progress" PRs incrementally. You can start a PR on an empty branch. You can apply a label "Work in Progress" and start discussion immediately. If you review a chunk of code the PR is pretty good about showing you what you've already reviewed versus what is new in the most…

GitLab employee here - that is true of GitLab too.

But I agree the bigger challenge than any tooling challenge is getting the team convinced that starting a PR/MR early is not only okay but also the way to operate. Creating physiological safety around it as well as making iteration a part of the day to day workflow is a challenge but an important one.

Re: On Code Review

#56

Post by WorldMaker »

somebodythere wrote: post #28
maccard wrote: post #26

Earlier quoted context omitted.

> Doesn't seem like such a big problem to make a PR with multiple commits, and then squash and merge into 1 commit after the review. I fundamentally disagree here. If you're reviewing code, the code that is pushed should be what is reviewed, not a draft of what is reviewed..

If you squash and merge shouldn't the final pushed code be exactly the same as what was reviewed?

Not necessarily. There's a lot of dark arts to merges and there's a lot that can be hidden in them. If you've never heard of, as just one for instance, the git rerere cache and campfire horror stories of how it can be corrupted consider yourself lucky.

Re: On Code Review

#57

Post by raducu »

pm215 wrote: post #23
eertami wrote: post #18

Earlier quoted context omitted.

> because I care about the convenience of the person that is actually interested in investigating a feature/bug 5 years from now and not of the lazy code reviewer now Doesn't seem like such a big problem to make a PR with multiple commits, and then squash and merge into 1 commit after the review.

Personally I think that is pessimal, because if I am the person investigating in 5 years time I want to see the change split up into multiple logical commits. So lots-of-commits-plus-squash-and-merge requires the developer to do the work of splitting up the feature implementation but doesn't give the benefit of having a usefully split set of commits to the future-debugging-person...

The change is a feature, it is by deffinition a "logical commit".

Like "migrated this JMS queue to kafka", that can span across tens of files.

Just because the commit is big doesn't mean it's not also logical and consistent.

In fact the reverse can be said -- if you split the commit in smaller commits (and your build is still green with each commit) -- each commit can seem contrieved, and good luck later on, when you try to understand how a piece of code works and how this xml/drl/properties file is connected to this piece of code if you have multiple commits.

If it's a single commit, you can usually narrow down your search tremendously and can easily see the logical deoendencies.

So for me, if it's a JIRA issue, I usually squash my commits before I push.

Re: On Code Review

#58

Post by gocartStatue »

V-2 wrote: post #19
KronisLV wrote: post #16

Earlier quoted context omitted.

> This implies the need for smaller pull requests maybe? This makes me feel like perhaps pull requests shouldn't always pull/merge the changes directly, but instead there should be something like the GitLab "Start a review" functionality, where you can queue up code comments, before submitting all of them at once, but for pull requests. As a Jira analogy, imagine the totality of the changes that you want to merge as…

"You don't actually want these changes to end up in the main branch before they are done." True - and it sounds like a textbook scenario for introducing a feature branch... this approach has worked well for me. The idea of queueing up "sub-requests" is interesting, but it comes at the cost of creating another layer of complexity (in my view).

Feature branches introduce more complexity than real value (bold statement, right)?

I really enjoy Dave Farley's videos on youtube - "Continuous Delivery" channel. A friend recently sent me this piece from Atlassian: https://www.atlassian.com/git/tutorials/comparing-workflows/...

Post reply on HN