Mistakes I see engineers making in their code reviews

Consistency and predictability are encoded in conventions which are someone's personal taste.

You don't get to have your own personal taste of the moment when you are reviewing; you have to broadly refer to the documented collection of tastes.

If there was one personal taste, this is achievable.

There is not. There are a variety of personal tastes and they all conflict with each other.

Then make that explicit topic during the setup instead of taking random commits hostage to whatever catches your attention this week.

There is a middle ground here though. I try, anymore, to make sure I’m very clear on which review comments are blockers for me and which - most - are style, suggestions, questions. Unless the code has some egregious problem I immediately approve it and write a comment saying “this is good by me if you fix blockers A, B”; that way they are not then blocked by me doing a second review unless they want it.

Maybe this is because I’m working somewhere where we don’t use stacked reviews though? So it’s a major pain for someone to have a PR open for a long time going through lots of cycles of review, because it’s tricky to build more work on top of the in-review work

FTA

> Review with a “will this work” filter, not with a “is this exactly how I would have done it” filter

I suppose there are edge cases where you could say "technically this will work, but when the system is close to OOM it will fail silently", but I would consider that to be a negative response for "will this work" rather than a case where you rubber stamp it.

Yes, ideally. Then again, I've caught plenty of bugs checking it out and running it locally.

The CI pipeline may not wholly be in your team's control. I've worked at places where a 'devops' team had too much control, liked to tinker all the time and often broke stuff. Nothing we could do - devops were gods and didn't take input from anyone.

Should I not do check it out locally and instead spend months and political capital arguing for more in the CI (which the PM would argue slows down velocity - and will win)?

Pragmatism and workarounds are often employed in non-bigtech enterprise companies...

edit: I also meant 'poke around' as in do a bit of informal QA. Again, your test and QA team might not be perfect...

There are times where it is reasonable, especially for bug fixes where there is a test to explicitly check that the bug is fixed. In that case, I might check out the code, revert the fix, and ensure that the test fails. There have been a few times I did that and the test still succeeded, so I can confirm that the test is not testing the right thing (in a very mature project, complexity or test settings can obscure what your test is actually doing, especially if it is an E2E test).

There can still be value in configuring your local IDE to emit more warnings than the centralized linter does, and looking at those warnings in your IDE.

This can help you identify additional pieces of feedback that you might have missed by just reading the code in a web interface.

If the code needs to work in 100 different environments it is likely that there are more bugs found, than if it is only tested in one standardised environment. The problem CI catches is, if that one environment isn't even standardised and reproducible.

> Personally I hate it when people just leave comments on my PR without explicitly blocking or approving it. To me it comes across as indecisive. Either you are fine with the changes -> approve.

I point out things that aren't functional blockers all the time. If I see there's a gap from the code itself, but maybe weren't considered (is there a ticket to deal with this? I'm not going to guess what it's named or where it is), I'll point it out. If it's not blocking, it's an approval from me, but someone else may disagree when they read it as well.

Not sorry that you think it's indecisive. This is technical feedback, which is separate from product feedback. You should still read your review comments and other people's review comments. If I was in charge of things to the point only my approval matters AND I was sure the author would respond, it would be an approval. Generally, this is not the case.

IMHO fundamentally the MR/PR author is the one requesting reviews (be it mandatory to merge or not), and they are the ones to decide if the comments are valid/require actions or not.

If the reviewer didn't accept your PR you already know they're expecting something more, and what it is will be in the comments.

I don't know if GitHub comments can be individually marked as optional or not, but even then the reviewer might not know the actual impact of what they're pointing at.

For instance if you're deleting a bit of feature that has been already disabled at the step before, a reviewer pointing it out as blocking might as well be lacking context. I find it more natural for a comment to be mostly factual and let you check if it needs a fix or not.

It's more work on your part, but it's also your PR (all of that within reason, and not being jerks)

What about: "It would save as effort, if you would do it like this, but having it in the current change is still better then nothing in case you are not willing to do the work."

The way I look at it, commenting without approval means "I don't approve (and here's why) but someone else can."

Blocking means "I don't approve and no one else should either."

Most people I know don't use it this way. Most people sign off on a review, with the expectation that the remaining comments will be addressed before pushing.

If they just push anyways, then post-push there are gonna be some awkward conversations. If it becomes common, other people would know to just not approve CRs for that person until they are pristine. It will basically just slow development speed down in the long run.

That's a bit unprofessional. The comment may be about a non-blocking issue. There is something wrong with the diff, and you should look into it.

Ignoring the comments is a tactic of a careless coworker. The diff may get merged and they can move on, sure. But come review time, they may find something else in their work environment is being rejected and removed.

But the MR/PR is not approved, just commented.

That's where I see little need for a "blocked" state, that's already what not approved should mean, short of explicitely saying otherwise.

I feel for you, I'd do whatever in my power to get out of that kind of toxicity.