Comment by diggan
19 days ago
I dunno, when I review code, I don't review what's automatically checked anyways, but thinking about the change/diff in a broader context, and whatever isn't automatically checked. And the earlier you can steer people in the right direction, the better. But maybe this isn't the typical workflow.
It's a waste of time tbh; fixing the checks may require the author to rethink or rewrite their entire solution, which means your review no longer applies.
Let them finish a pull request before spending time reviewing it. That said, a merge request needs to have an issue written before it's picked up, so that the author does not spend time on a solution before the problem is understood. That's idealism though.
The reality is more nuanced, there are situations where you'd want to glance over it anyway, such as looking for an opportunity to coach a junior dev.
I'd rather hop in and get them on the right path rather than letting them struggle alone, particularly if they're struggling.
If it's another senior developer though I'd happily leave them to it to get the unit tests all passing before I take a proper look at their work.
But as a general principle, please at least get a PR through formatting checks before assigning it to a person.
>> And the earlier you can steer people in the right direction, the better.
The earliest feedback you can get comes from the compiler. If it won't build successfully don't submit the PR.