3 ms·

> Mostly I won't even look at a review until CI passes. I don't want to look at your code if some static analysis in CI would find issues. > but we have a lot

> Mostly I won't even look at a review until CI passes. I don't want to look at your code if some static analysis in CI would find issues. > but we have a lot of static analysis that we run as part of CI, some of it standard some of it custom for people who violate our code style in some way. Yeah, but why would issues that static analysis find stop you from doing the review? I never review things that automated tools can find, I care more about reviewing things only a human could review. "Does this make sense here?", "Is this decoupled enough/too much?", "Does this work well with the overall architecture?", "Does this test test the right thing?" and so on. I wouldn't spend my review time on spellchecking people or pointing out issues like that, that's for the tooling to do. And even if the CI fails because of some analysis, PR author fixes it, every review comment should still be applicable, otherwise you're just doing robot work.

bluGill 3y ago
Sometimes static analisys finds something that needs significant code changes to fix. Other times it will tell you use an algorithm to do this instead of a hand written loop and so the code is a lot easier to understand after. Most of the time it doesn't matter, but I don't want to waste my time in the few cases where it does.
plorkyeran 3y ago
I don't strictly wait for CI to pass before reviewing, but I don't think it's a crazy idea. There's no point in taking the time to understand a set of changes and evaluate the design and such if it doesn't actually work in the first place and needs to be reworked. If you have something that's easy to test locally and the CI checks are just a backstop then the initial PR should nearly always pass, but if your CI checks are much more thorough than a developer can do manually then a PR initially being in a broken state may be a routine occurrence.

bluGill 3y ago

Sometimes static analisys finds something that needs significant code changes to fix. Other times it will tell you use an algorithm to do this instead of a hand written loop and so the code is a lot easier to understand after. Most of the time it doesn't matter, but I don't want to waste my time in the few cases where it does.

plorkyeran 3y ago

I don't strictly wait for CI to pass before reviewing, but I don't think it's a crazy idea. There's no point in taking the time to understand a set of changes and evaluate the design and such if it doesn't actually work in the first place and needs to be reworked. If you have something that's easy to test locally and the CI checks are just a backstop then the initial PR should nearly always pass, but if your CI checks are much more thorough than a developer can do manually then a PR initially being in a broken state may be a routine occurrence.