81–90 of 122 posts
Earlier quoted context omitted.
I have an open PR from my first week. Not a single review. I’ve merged entire features since then…
Do you specifically ping people to review? At my company we've got a habit of not only assigning reviewers in GitHub but also pinging them in slack.
Earlier quoted context omitted.
I agree that the coding expectations are fine-- and I'm not saying you do this-- but conveying annoyance through pedantic, overly nitpicky, or snarky junior code reviews is a management and mentorship failure. In any field, someone consistently acting like a know-it-all is indulging their own emotional shortcomings under the guise of enforcing good practices. Firstly, if this is a brand new junior and they didn't hav…
> but conveying annoyance through pedantic, overly nitpicky, or snarky junior code reviews is a management and mentorship failure Completely agree. If I get a chance, I almost always try to have a conversation (Zoom, in person) instead of writing large walls of text. It's definitely discouraging to, really anybody, to see your review get slammed by someone. Usually, if I see a common pattern or something is just whol…
Whenever there is a squabble over style in any PR in my team, I ask them to merge, decide afterwards on a single solution and then write a linter rule. Often a custom rule is necessary.
IMO the worst codebases to work on are those that have nitpickers that change taste all the time. Newbies join, try to "read the room" and find a lot of code that looks good, so they use it as a template. Only to be nitpicked because "we do things different now".
Earlier quoted context omitted.
To be fair, it's not always a bad idea to present such ideas. However you need to make the comment resolvable, or better yet, create an issue for refactoring, add a small "fyi" comment and let them resolve it when they read it. This way they are aware of possible upcoming changes to this code but the MR/PR is not blocked.
I think "fyi" comments are valuable in PRs. Often all I want is for my team to approach similar problems differently in the future, not necessarily to refactor the immediate code. Or at the very least consider alternative approaches
How to make your colleagues miserable 101. 1. open a cr, 2. have your inexperienced friend approve it, 3. have the lead eng add 15 comments they caught, 4. merge the code anyway despite it breaking the core feature that’s being merged in parallel because you can’t just follow the design of the person who thought everything through and decided you knew better, 5. waste 8 more hours of your lead while they pair program…
How does a company even allow a person that does this to work there?
I don't have an adequate answer.
Earlier quoted context omitted.
Code reviews seem like a hellscape of "could do X". Like man it's code, there's an infinite amount of "could do" with an infinite amount of contexts. I found google's advice to be pretty good https://google.github.io/eng-practices/review/ while they give a lot of good advice / suggestions, they also make a point that there aren't a lot of hard stops and generally if the code works and isn't horrendous you let it go.
I feel like if you're looking for design feedback during the code review that's already way too late. There should be much more communication going on before the coding even happens.
I think a nice little "Hey I saw this that doesn't match this design doc here, but this all looks good regardless." type reminder would be ok. Obviously being diplomatic / soft skills is huge and the google doc addresses that a great deal.
But yeah especially if this is a non-trival change / lotta work ... not time to re-invent the wheel.
Code review is a flawed process, especially for junior engineers. You spend time and effort, maybe days of work, getting this to work, and then some know-it-all leaves a bunch of nit-picky comments or tells you you're doing it wrong. I tell most of the juniors I work with to work defensively against this using a few strategies: - smaller PRs. Break the work up any way you can. Make small tickets or submit your PRs wi…
I understand that code review has a place in enforcing code quality and training junior programmers. Here's what I would suggest. When you hire someone, make sure somebody is assigned to reviewing their code for the first few months. Once they've demonstrated their ability to write good code, from that point forward you trust them not to screw things up. In addition, foster a culture where people are open to criticism and proactive about fixing things. If someone sees a problem with someone else's code, they can either let them know informally, or just go in and fix it themselves. I think this approach would lead to much less red tape and much happier and more productive programmers.
Code review is a flawed process, especially for junior engineers. You spend time and effort, maybe days of work, getting this to work, and then some know-it-all leaves a bunch of nit-picky comments or tells you you're doing it wrong. I tell most of the juniors I work with to work defensively against this using a few strategies: - smaller PRs. Break the work up any way you can. Make small tickets or submit your PRs wi…
It also brings significant additional overhead - generally every merge to master has to be tested individually, since there's no guarantee all PRs will be merged before release goes out.
Earlier quoted context omitted.
> Bonus if the vague comments relate to some refactoring that you'd like to do I am extremely ashamed to say I have left a comment like this before. At the time, I was frustrated about never being able to discuss anything over a call. There is no excuse, though.
To be fair, it's not always a bad idea to present such ideas. However you need to make the comment resolvable, or better yet, create an issue for refactoring, add a small "fyi" comment and let them resolve it when they read it. This way they are aware of possible upcoming changes to this code but the MR/PR is not blocked.
This way the value of the pr process is captured: preventing obviously bad changes from reaching prod and two-way knowledge sharing.
Setting a higher bar for reviews more often than not blocks people for days without good reason.
Earlier quoted context omitted.
What, those are rookie numbers! I opened a pull request 13 business days ago. I even rebased it multiple times when conflicting code was otherwise merged. There’s been no feedback whatsoever. Everyone even remotely involved is now on vacation. (It’s not like I care either, the project sucks anyway.)
The best is when the only possible reviewer is your Engineering Manager because you have no other engineers in your domain on the team.