DEV Community

loading...
Cover image for What's your best PR etiquette tip?

What's your best PR etiquette tip?

Waylon Walker
Data Engineering with python, kedro super user.
・1 min read

What's your best PR etiquette tip? From the standpoint of reviewee or reviewer, your choice.

Discuss πŸ‘‡

Discussion (12)

Collapse
jmfayard profile image
Jean-Michel Fayard πŸ‡«πŸ‡·πŸ‡©πŸ‡ͺπŸ‡¬πŸ‡§πŸ‡ͺπŸ‡ΈπŸ‡¨πŸ‡΄ • Edited

As a reviewer, my goal during a PR is to ask questions, not to be a gatekeeper.
I approve the pull request once I've done that.
I trust the reviewee to find good enough answers, or to ask for help if needed.
I know I have succeeded if in a next PR my fellow programmer ask herself the same questions.

I think often about this article from @daedtech : How to Use a Code Review to Execute Someone’s Soul

Collapse
sargalias profile image
Spyros Argalias

Nice. I think it's good to have trust that your colleagues are good. Good points in general.

Collapse
jmfayard profile image
Jean-Michel Fayard πŸ‡«πŸ‡·πŸ‡©πŸ‡ͺπŸ‡¬πŸ‡§πŸ‡ͺπŸ‡ΈπŸ‡¨πŸ‡΄

That's the point, pull-requests were invented for the open-source model where you have zero trust with the first time commiters. Instead of emulating a zero-trust environment, build trust!

Collapse
waylonwalker profile image
Waylon Walker Author

not to be a gatekeeper.

Fantastic tip

Collapse
sargalias profile image
Spyros Argalias

Be very polite. Remember that people are sensitive and getting a PR comment can make people very defensive.

This means to talk about the code, not about the person. E.g. "the code is doing X" rather than "you wrote X".

I also try to write less assertively. For example, I'll say: "I think scenario X is likely to happen. In that case, there might be X problem. What are your thoughts (or, if I'm very confident the issue will happen, I'll say "can we update the code to prevent this")?". I won't say "the code fails under scenario X. Please fix it".

Feels a bit like treading on eggshells, but I think that's for the best.

Collapse
waylonwalker profile image
Waylon Walker Author

Feels a bit like treading on eggshells, but I think that's for the best.

for sure. I somehow found myself in the position of team lead with very little of my code going through review. Now I review lots of code, so its hard for me to find words for things.

I have a bunch of linters and checkers running so pedantic stuff is already taken care of. I have been known to be very hyper focused on robustness and reliability. I am very focused on the next person being able to read, understand and change the code. If you have set the next person up to fail by connecting two things that shouldn't be or not connecting two things that should be I'll call that out.

Collapse
sargalias profile image
Spyros Argalias

Very nice. Yeah automating the little stuff makes everything better. Less time wasted on it and you can focus on the important things more.

The rest sounds like you're reviewing code as it's meant to be reviewed :)

Thread Thread
waylonwalker profile image
Waylon Walker Author

this comment made my day!

Collapse
s_aitchison profile image
Suzanne Aitchison (she/her)

Conventional comments I think are hugely helpful - for example, instead of commenting on a PR with:

this could be extracted to a helper function

This would be more useful:

suggestion (not blocking): this could be extracted to a helper function

It's really hard to receive PR reviews with lots of "general comments" - there's a lot of mental processing and effort involved in establishing what needs responded to, what's a genuine blocker, and what's just an idea being put out there.

The other thing I would say is to be sure to comment on positive things, as well as things you would change/suggest. It just helps transform the PR review from a "barrier to merging" into a genuine conversation about the code and approach, which everyone can learn more from.

Collapse
sargalias profile image
Spyros Argalias

I agree. I've started using conventional comments lately and I love them.

Collapse
cdthomp1 profile image
Cameron Thompson

When I review, I try to follow some techniques outlined in this article:
mtlynch.io/human-code-reviews-1/

it’s a time for improvement and learning, not a time to degrade someone. I look for ways to use the PR to teach, which helps keep sports up in the long run :)

Collapse
waylonwalker profile image
Waylon Walker Author

Love the idea of a PR being time for teaching not degrading.