Pull request review mistakes • Hacker News Board

Re: Pull request review mistakes

#11

Post by glebm » Sat, Jan 28, 2017, 1:48 PM UTC

throwawayish wrote: post #10

glebm wrote: post #8

Earlier quoted context omitted.

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

> most linters for most languages are unable to do this,

In the long run improving the linter is better for everyone. The examples given by the OP as infeasible are feasible.

Focus on the design over trivial style issues. Your style guide should be 95% covered by the linter. If it isn't, fix your linter.

> Broadly missing a point

My points is: Underestimating static analysis will lead you to the wrong conclusions.

Re: Pull request review mistakes

#12

Post by glebm » Sat, Jan 28, 2017, 1:52 PM UTC

throwawayish wrote: post #10

glebm wrote: post #8

Earlier quoted context omitted.

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

> some instances of these decisions where it is doubtful that any linter would be able to lint it.

Such issues are rarely style issues. They are more in the design territory where the focus of code reviews should be.

Re: Pull request review mistakes

#13

Post by SoulMan » Sat, Jan 28, 2017, 3:09 PM UTC

One of the problems we have faced when multiple engineers ding exactly similar work (e.g. Create and Update operations of a CRUD module) but in a very different class and method structures. Now whoever makes it to master first becomes the standard and others needs to modify to confirm to that style. However everyone's style is correct in some sense but reviewer needs to make sure all code is written by one team not that one file has factory pattern and another one has switch case .

Re: Pull request review mistakes

#14

Post by quantumhobbit » Sat, Jan 28, 2017, 3:28 PM UTC

SoulMan wrote: post #13

One of the problems we have faced when multiple engineers ding exactly similar work (e.g. Create and Update operations of a CRUD module) but in a very different class and method structures. Now whoever makes it to master first becomes the standard and others needs to modify to confirm to that style. However everyone's style is correct in some sense but reviewer needs to make sure all code is written by one team not t…

Not that there isn't value in having everyone follow the same patterns, but I find it gets emphasized over just getting work done a lot of the time. I've seen projects waste so much effort making the code uniform instead of delivering features. If the code works and is readable, who cares if it varies somewhat from similar code.

Re: Pull request review mistakes

#15

Post by crdoconnor » Sat, Jan 28, 2017, 3:44 PM UTC

SoulMan wrote: post #13

One of the problems we have faced when multiple engineers ding exactly similar work (e.g. Create and Update operations of a CRUD module) but in a very different class and method structures. Now whoever makes it to master first becomes the standard and others needs to modify to confirm to that style. However everyone's style is correct in some sense but reviewer needs to make sure all code is written by one team not t…

This suggests you need DRYer code.

Re: Pull request review mistakes

#16

Post by rocky1138 » Sat, Jan 28, 2017, 3:55 PM UTC

One thing that's improved our results is asking to see the changes actually working. The person has to demo them live to the reviewer. Many times a small bug or hiccup occurs which would never have been seen by comparing diffs.

Re: Pull request review mistakes

#17

Post by rocky1138 » Sat, Jan 28, 2017, 3:57 PM UTC

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…

For me, a lack of adherence to our conventions is a smell that not all i's have been dotted and t's have been crossed. If the style is poor, I start to think the logic may be suspect, as well.

This is doubly true when delinting and styles are automatic via git hooks.

Re: Pull request review mistakes

#18

Post by hobarrera » Sat, Jan 28, 2017, 4:37 PM UTC

rocky1138 wrote: post #16

One thing that's improved our results is asking to see the changes actually working. The person has to demo them live to the reviewer. Many times a small bug or hiccup occurs which would never have been seen by comparing diffs.

This sounds like it might work for PRs for huge features, but not every PR. Coordinating a live demo takes more time than it takes to write entire PRs for most cases. Especially if more than one person needs to see it.

Re: Pull request review mistakes

#19

Post by neandrake » Sat, Jan 28, 2017, 4:44 PM UTC

hobarrera wrote: post #18

rocky1138 wrote: post #16

One thing that's improved our results is asking to see the changes actually working. The person has to demo them live to the reviewer. Many times a small bug or hiccup occurs which would never have been seen by comparing diffs.

This sounds like it might work for PRs for huge features, but not every PR. Coordinating a live demo takes more time than it takes to write entire PRs for most cases. Especially if more than one person needs to see it.

Rather than a demo, we often ask for screenshots or gifs, which has had similar effect of catching some things earlier.

Re: Pull request review mistakes

#20

Post by hobarrera » Sat, Jan 28, 2017, 4:53 PM UTC

LandoCalrissian wrote: post #4

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…

I really try to filter out how the code review / testing works when interviewing for a job. Whenever I get to the "do you have any questions for us" part, I start asking about their workflow, review process, and tests coverage, as well as several more practical questions on how they deal with it ("what would obviously get a PR rejected").

The obvious ones I'd discard are the ones that don't use a VCS (I don't think I'd take a non-git one either, and CSV is definitely a no-go).

Sometimes it's interesting that no mention of tests is made when asking these questions, or style guides maybe. It really give you a taste of the "code culture" for that employer.