Regular code-reviews are supposed to catch errors, bad style, or bad architectural decisions early on in the process. For it to work, however, everyone should be super disciplined and spend a lot of brain-power reading through each diff. Usually, it's really hard to dig into someone's code until you're fully int the cotext (read: working on the same piece of code). On the other hand, I often find myself in the situation when I'm dealing with someone else's code and think, "Oh god, how could I even approve this PR?". The next thing I usually want to do is let the team, and specifically, the code author, know that this is how we should NOT DO things. The problem, however, is that the code-review window has closed and there's no other standard place (besides GitHub PRs) to comment on the code.<p>I'm wondering if commenting on the code AFTER it has been merged is standard practice on some project and if there are any good tools for that.
Why can't you say "this is how we should not do things" during the code review window? You should have a policy in place that all comments need to be addressed before approval, and with your comment in there (e.g. "we should not hardcode api keys" or "pull default params from the config file, not ENV variables"), others should not approve. Your CI process should require approval before merging. Now if it makes it past review and approval and gets merged, you can create a "tech debt" issue to refactor. You should also have a meeting of the minds to put these standards in a style guide and make sure it is followed when reviewing PRs.
Just file a new bugtracker ticket or whatever that says "Clean up foo.js, it does Bad Thing A and Bad Thing B"?<p>There are processes for sustaining code architecture reviews and paying back tech debt, but if you're in a team that's merging obviously terrible code you might not be large enough to want that kind of structure.
I have the same problem at my job and a very similar process (biggest difference is that we require unanimous consent before merges). My overall opinion towards code reviews is that they don’t work, software should not be seen as a static entity that is released and completed (like a building), but as something that requires constant upkeep (like an garden). Since our process was established 20 years ago (while I was in elementary school) time has been cemented it by bureaucratic inertia. So my approach is not what you can do to your process, but what you can do as an individual within your existing process.<p>The idea behind my approach is to convince the person to do the right thing on their own. First step is to mentally prepare, as if you were an actor preparing for a roll as a villain. Your attitude should be “any code not written by me is garbage, and should be completely rewritten.” This attitude (which you should not adopt in your general professional life) will help you get through this ordeal. Next step is to poke as many holes in the code under review as you can in the time you have. Don’t stop on the diffs, do it for the whole code base. If the person touched the code they are responsible for the whole thing. Send as many issues as you can back, cc their management. This will make some people angry, but it will be temporary and most importantly buy you time.<p>Management will be aware that their are issues with the merge, but they don’t have the time to review your comments and understand the issues, they have other problems. They don’t want doubt in the work going out and will do anything to make themselves feel confident. They will put their trust in you and will not allow the merge until you say it’s good. You are the gate keeper, you hold the key.<p>Now it’s time for the real code review. The person having the review now depends on you. This is where you get as friendly as possible. Complement sandwich works here “I like your code, there’s just a few things that need to be changed, this looks like it’s going to be a slam dunk, we’re almost there, good job so far buddy.” The most important thing now is to identify specific issues that have bounded solutions (not wild goose chase ordeals). The more specific and more bounded, the requests are, the happier people will be and the faster people can move on.<p>The whole idea here is to develop an understanding with people. You’re going to tell people what you expect, and make it as easy as you can to carry it out without significant impacts to their schedule. As soon as they deliver on your requests, you approve. You go to management and say the code looks a lot better and you approved the peer review. Everyone looks good, everyone is a winner here.