Can't relate. I take code reviews as possibly the most important part of my job as a developer. Suggesting extra tests, thinking about unintended side effects, and yes, aiming for consistency and readability, without being picky on style choices.
I trust my colleagues to do the same (and they often do).
I can't imagine working in an environment where this is a theater.
A lot of developers are working in hellish companies, micromanaged and beaten down into cogs.
One very valuable skill for those of us who have experienced productive collaboration is learning how to introduce it to new places.
Sometimes that means telling the executives that their promotion process is making them less successful. Sometimes it means wandering around PRs leaving useful positive comments to seed the idea that PRs can be useful. Sometimes it means pointing out tests only when there is a bug, so people can experience how great it is to follow the practices that keep us from introducing bugs in the first place.
I wish that more CS programs would explicitly teach their students critique skills, the way art and music and english and math programs do. But until then, we're counting on engineers getting lucky and landing in a functional workplace like yours.
It sounds like a good job where the most important part is finding other people’s mistakes.
Though I do appreciate the shoutout to adding tests in CR. But returning a PR solely because it doesn’t have tests, is effective, but a little performative too. It kind of like publicly executing someone, theirs gotta be some performance for it to be a deterrent. If something doesn’t have tests my review is going to be a very short performance where I pretend read the rest of the code. Then immediately send it back.
It's interesting: the original definition of "performative" is "a speech-act that changes something about the world".
And that basically describes all of programming: we are building metaphors that will run electricity at a higher or lower voltage, and be translated again into something meaningful to a different human.
In many ways, all we are doing is performing. And that is some of what makes this job challenging: the practices that build software well are all just ways of checking how humans will interact with the ones and zeros we've encoded.
Returning a PR because it doesn't have tests means that code will have automated validation, which is a real change. It also means the code will be written in a testable way: too often we don't realize we wrote code in a way that is hard to test unless we try to write the tests. And on a larger level, it means that this team of engineers will learn and use the practices and tools that lead to testable code and effective tests and more easily-changeable code.
It makes total sense to not keep reading if there aren't tests, because adding the tests can be expected to change the code. But just because that is a performance doesn't mean it doesn't profoundly change the world.
> It sounds like a good job where the most important part is finding other people’s mistakes.
And reviews are not that. Systems are complex, and having a mental model of complex systems is difficult. Everyone has blind spots. A fresh pair of eyes can often spot what who was coding would not.
> But returning a PR solely because it doesn’t have tests, is effective, but a little performative too.
And this is not what I said. I spoke of suggesting extra tests. A scenario that wasn't covered, for example.
i think you would benefit from some time in an organization that was not shitty