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

Pull request review mistakes

blog.scottnonnenberg.com

Post reply on HN
Print view View original

1–10 of 33 posts

Re: Pull request review mistakes

#2

Post by zachrose »

I'd be very interested in a tool for diffing that gave some broad overview of how things have moved around. Otherwise, the kind of refactoring where you break things apart just creates a pull request that's harder to get oriented in. Which is a shame because a harder to gloss code review can disincentivize the changes that are needed most.

Conversely, it's just so easy to add an if statement there or any early return here, and over time you've got a 400-line method with like 100 branches and an Avagadro's number of possible paths. But you look at a two-line diff and say, "Hey, this looks reasonable."

Re: Pull request review mistakes

#4

Post by LandoCalrissian »

The frontend complexity one rings very true. I know they are largely talking about CSS changes in the article, but I think sometimes experienced developers can just discount the frontend as a whole.

At my job my coworker and I did a major refactor on part of our frontend codebase. Probably 2,000 lines of code were changed and the only comment on the PR was for additional unit tests on one method we had touched on the backend. We brought it up multiple times in standup that this really needed to be reviewed properly and we got radio silence. The PR was just passed through.

Your entire platform really does matter, it's important to make sure all things work. Not just what you understand.

Re: Pull request review mistakes

#5

Post by hnruss »

My team has benefited significantly from diligently reviewing pull requests. It may seem like a waste of time at first, but I can't count how many bugs we've caught this way. Team productivity is also increased due to knowledge-sharing, as we have to understand more parts of the code in order to properly review it. Reviews typically take between 10 mins and 4 hours, depending on the size of the PR. We've also improved our competence with Git, as we have tried to make the series of commits in a PR more understandable. PRs have actually increased work satisfaction on my team, as we get much more feedback on our work this way. Usually feedback is positive, but even when it isn't, we work at it together until the PR looks good.

Re: Pull request review mistakes

#6

Post by danpalmer »

hnruss wrote: post #5

My team has benefited significantly from diligently reviewing pull requests. It may seem like a waste of time at first, but I can't count how many bugs we've caught this way. Team productivity is also increased due to knowledge-sharing, as we have to understand more parts of the code in order to properly review it. Reviews typically take between 10 mins and 4 hours, depending on the size of the PR. We've also improve…

I have a very similar experience. Another thing we do is to align our style guide (and automated linting) with code review, mostly with the intention of reducing diff noise and therefore mental overhead when reviewing. The ability to align in this way depends on the language, but we've had a fair bit of success doing this in Python.

Re: Pull request review mistakes

#7

Post by blauditore »

About 4. Style over substance:

Coding style (apart from formatting) can be crucial for understandability, so it's important to have a critical look at it when reviewing. For instance, consider the following snippets:

  void myMethod(FooBar arg) {
    if (arg != null) {
      arg.foo(something);
      for (int i = 0; i
vs

  void myMethod(FooBar arg) {
    if (arg == null)
      return;

    arg.foo(something);
    arg.bars().forEach(baz);
  }
Even though they're functionally identical, they look wildly different. These things can hardly be judged by an automatic process like linting, so it's important to manually do that.

Re: Pull request review mistakes

#8

Post by glebm »

blauditore wrote: post #7

About 4. Style over substance : Coding style (apart from formatting) can be crucial for understandability, so it's important to have a critical look at it when reviewing. For instance, consider the following snippets: void myMethod(FooBar arg) { if (arg != null) { arg.foo(something); for (int i = 0; i vs void myMethod(FooBar arg) { if (arg == null) return; arg.foo(something); arg.bars().forEach(baz); } Even though th…

> These things can hardly be judged by an automatic process like linting

Both of these (if vs guard clause, loop vs each) are enforced by the Rubocop linter in Ruby.

Re: Pull request review mistakes

#9

Post by throwawayish »

hnruss wrote: post #5

My team has benefited significantly from diligently reviewing pull requests. It may seem like a waste of time at first, but I can't count how many bugs we've caught this way. Team productivity is also increased due to knowledge-sharing, as we have to understand more parts of the code in order to properly review it. Reviews typically take between 10 mins and 4 hours, depending on the size of the PR. We've also improve…

If there are multiple releases/branches then I also recommend to very carefully review merges between branches, even files/regions where git (or whatever SCM is used) merged automatically with no conflicts. I've personally seen multiple incidents now were through review bugs and security issues where found in merges that ranged from subtle to catastrophic. I even saw one catastrophic security issue introduced into a file git merged with no conflicts.

(However, more issues are typically introduced by manual conflict resolution, since we humans are also easily confused when doing it. Both are a problem relatively independent of language, although some specific instances might be caught by a compiler or tests. If the latter don't catch it, it may be a sign that some tests are missing.)

Be careful.

Re: Pull request review mistakes

#10

Post by throwawayish »

glebm wrote: post #8
blauditore wrote: post #7

About 4. Style over substance : Coding style (apart from formatting) can be crucial for understandability, so it's important to have a critical look at it when reviewing. For instance, consider the following snippets: void myMethod(FooBar arg) { if (arg != null) { arg.foo(something); for (int i = 0; i vs void myMethod(FooBar arg) { if (arg == null) return; arg.foo(something); arg.bars().forEach(baz); } Even though th…

> These things can hardly be judged by an automatic process like linting Both of these (if vs guard clause, loop vs each) are enforced by the Rubocop linter in Ruby.

Today in: Broadly missing a point.

It's good and nice that there is one linter for a particular language that catches 100 % of these issues in an example, but that doesn't do much to change the fact that most linters for most languages are unable to do this, and that there are quite some instances of these decisions where it is doubtful that any linter would be able to lint it.

Post reply on HN