91–100 of 122 posts
Earlier quoted context omitted.
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
Also nit:
Although I much prefer to add automatic code formatting and linting and stuff to codebases to dramatically reduce the occurrence of 'nit:'
Earlier quoted context omitted.
The problem with this advice is that, at least in this story, if we're taking it at face value, the antihero programmers didn't learn.
I read it as weren't given the opportunity to learn, and didn't a have a mentor to teach them.
There were no adults in the room.
Earlier quoted context omitted.
I dunno mate, I think adults should be able to handle being chortled at. You're responsible for your own emotional state. If you've done something laughable (or even if you haven't), then don't be surprised when people laugh. Taking it personally is optional and not recommended.
I agree that people shouldn't be too thin-skinned, it's still important to stay civil and supportive. Being able to clearly and unambiguously convey that a change sucks while not hurting the other person's feeling is a useful skill to practice. (It's okay to fail at it, too, as long as you're aware that that's almost always a weakness.)
I'd rather work somewhere the burden of not being offended is placed on the listener, not the speaker. Somewhere we can just say what we think.
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
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.
Earlier quoted context omitted.
YMMV but i work at a place where * code review is expected responsibility, so everyone participates in every part of it regularly, so they are also incentivized to keep the process sane * we have an auto linter and we recommend saving on fix specifically so no one argues about useless style nits * CR back and forth is measured in minutes or hours so you are not waiting days to resolve someone’s drive by comment * CR…
> * CR feedback always has a specific action item that is easy to address Then CRs are pretty much pointless. The feedback I want as a senior developer is the complex stuff and that is half of the time not easy to address. The trivial stuff I usually, but not always, spot myself when checking the code before sending it for a review.
If you are doing system/algorithm design in the code review, it’s not meant for that.
The action item can also be “can we create a issue to track and discuss this further”
Earlier quoted context omitted.
Review can also effect maintainability; the code should ideally be parseable by other people on the team with little context of the specific project. And it should ideally spread knowledge of the code to at least one other person.
But in this case, I think it might be related with the background/knowledge/ways of coding difference between the maintainers?
A lot of stuff is codifiable as well. Shove your lint config in the repo, since you should all share a code style.
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 often add multiple comments and then approve the pr. My main question is always: does this make the codebase overall better or worse for some definition of good? If that bar is met, iterative improvements should not block deployment/merge. 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 t…
(honest question, please do not downvote): Why the tech industry still relies in the CR as a quality assurance/correctness process?
There are a lot of qualitative reasons: it helps spread knowledge of changes in the code, it helps maintain a consistent style, shares knowledge, etc. However when it comes to quality assurance and correctness (if these terms collectively mean, preventing defects ) then there's little evidence that it is an effective practice [0]. If the changes proposed are less than a couple hundred lines of difference and the revi…