There are many ways to solve any problem. If I reviewed every PR with the standard "is this exactly how I would have done it", I too would be destroying every PR I review. And I'm not even that good at coding - I'm the type of generalist who cares about things like the business beyond the code base.
What really matters is whether the code is solid and whether it will actually cause problems. Just because a PR isn't character-for-character how I'd write it doesn't mean it's bad. Based on the author's experience with the fired employee's replacement being just as bad, my guess is he was over defining goodness and raising the bar too high.
Bad PR to me means: bad runtime complexity, bad formatting, bad security, bad memory use, bad understandability. If it meets all those bars, which for a crud app is usually easy, then it should probably pass. I'll sometimes leave a "I would have done _x_ differently" comment on matters of taste _if I think it will help the other developer_.
> If I reviewed every PR with the standard "is this exactly how I would have done it",
This, too many people think their way is the only way. It only takes one cancelled project or one company going bankrupt (and 100,000s loc going "poof") to understand this work we do, really doesn't matter all that much in the end.
I've spent over 6 years working on 5+ legacy projects that had millions of lines of code and maintenance teams 30 times smaller than the army of contractors who developed them.
And I've learned all the small nit-pick maintainability issues made no difference. Whether or not there was a style guidelines, and whether or not it was enforced made little difference.
Things that really made a big difference were giant bone-headed architectural decisions like "just save persist this data as serialized text", "lets write all the business logic in stored procedures", "building the software on a terrible platform".
And having some type of identifiable domain model helped a little too.
It is a really humbling thought that every single one of our entire life's work as developers is one unfortunate astronomical event away from vanishing as though it never existed. Kind of puts it all into perspective really.
What really matters is whether the code is solid and whether it will actually cause problems. Just because a PR isn't character-for-character how I'd write it doesn't mean it's bad. Based on the author's experience with the fired employee's replacement being just as bad, my guess is he was over defining goodness and raising the bar too high.
Bad PR to me means: bad runtime complexity, bad formatting, bad security, bad memory use, bad understandability. If it meets all those bars, which for a crud app is usually easy, then it should probably pass. I'll sometimes leave a "I would have done _x_ differently" comment on matters of taste _if I think it will help the other developer_.