I appreciate and agree with your comment about taste, but that's not a reason to block a merge. It's a teachable moment, though.

The reasons to block merging code should be explicitly and clearly stated in your Coding Standards documentation.

The sort of clean that a functioning restaurant is, is something many people don't achieve at home.

I wonder sometimes if these arguments in software are less of an issue of lack of care for craft, and more just a fundamental misunderstanding that the sorts of arguments and compromises one makes with a roommate over what state the apartment is allowed to be in are simply not analogs for a professional setting.

I get the very distinct feeling sometimes that my mis en place rules are being received as if I'm whining about whether coasters get put away when not in use, and how long old magazines get left on the couch.

Nearly all of the fastidiousness I can manage in a day I save for work, so I'm very familiar with the other side of this conversation outside of the office.

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.

Consistency and predictability could also be considered a sign of maturity. Maturity is something to be observed, not declared.

It also depends where you draw the line. For example, is dependency injection a matter of taste or a fundamental design decision for a project? What about "defensive" coding practices, like `0 == x` over `x == 0` in C?

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.

c - it represents a complete failure to adhere to a standard of coding that we all approved as a group.

Any of these things are a negative commentary on your process, or the members of your team.

When one finds oneself standing in a hole, it is important to stop digging.

Same here, I have a Cargo.toml in which I painstakingly went through every lint and decided whether it made sense to me.

That is a bit overzealous. It makes sense for CI where you don't want to waste running tests on an already failing build, but for local builds you probably want to allow warnings and be able to run tests, lints, and formatting checks separately.

I’ve heard this argument before but I don’t get it. It’s trivial to setup your code editor to format on save. Why do people not do that but rather complain about files linter checks on CI?

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.

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)

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

Most places I’ve worked it’s fine to just comment. By approving or blocking, you signal to the team that you’re taking on the role of “the reviewer” for the code. Approving means that you have enough context to be confident in the change and stake your reputation on it. Blocking means you commit to doing subsequent rounds of review and eventually approving when your concerns are addressed (or you’re asserting a firm preference that this change should not happen).

Maybe you’re not the best person to approve the code, or you just don’t have time to commit to the reviewer role at this second, but you’ve still spotted something that you think will help the person writing the code; so, leave a comment.

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

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

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 can't decide if github is better or worse for making you 'resolve' all conversation whether you choose to do anything about them or not.

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

The approver of a PR shares some responsibility in the case where the code causes production issues.

So look at the code and decide if you're willing to defend it if someone says, "Who approved this for production?" If you did your due diligence, thought the tests and the code were reasonable but some obscure interaction caused problems, you didn't have a way to know that.

If the code is just full of bad code smells and that's what blew up, then your defense is flimsy.

Production issues will happen. But they should always be the confluence of two or more errors resulting in a bad situation. Single cause failures are inexcusable.

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.

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

Some people don't know what responsible means.

To some people, "I take full responsibility," means, "I will publicly admit to being wrong," instead of, "I will do everything in my power to clean up this mess that is my creation."

This sounds like you're making an argument against pull request descriptions in general. Like you just want to see code changes and have a meeting about it.

In those situations, the more productive option to course-correct is to talk to the change author in a meeting/chat instead of just volleying off a tsunami of comments about various minutiae in the change IMO.