Polite pull requests: A guide to emotionally intelligent code reviews
This is a live streamed presentation. You will automatically follow the presenter and see the slide they're currently on.
This is a live streamed presentation. You will automatically follow the presenter and see the slide they're currently on.
A guide to emotionally intelligent code reviews
[Image: A rooster in a meadow.]
Follow along for some presentation fun
(results may vary; speaker is not liable if presentation is not actually fun)
First edition
[Image: The cover of the book Eloquent JavaScript.]
Senior Software Engineer
"Yves" sounds like "Eve" or "Steve."
JavaScript is my love language.
Devetry since October 2021.
G4m3rz with poor gaming skills.
[Image: Portrait of the speaker.]
Always looking for parenting advice.
A request submitted with the intent to merge changes into the codebase once they have been approved.
[Image: Example of a pull request with code changes and a conversation.]
But not the one you're thinking about.
Solves a lot of problems preventatively.
Because I want you to talk nicely to me.
How's my driving?
🇫🇷
Tried a different format for this presentation:
fictional examples first instead of theory.
We might not go through the entire presentation but that's not a problem. I would prefer a good discussion. Slides are sorted by importance.
This page intentionally left blank.
Sorry, I've always wanted to do that at least once in a presentation. 😅
That function is wrong.
function getBookTitle(language, edition) {
const bookTtle = `Eloquent ${language}: ${edition} edition`;
return bookTitle;
}
It seems like there is a typo in the constant name `bookTtle`.
Avoid using "I think," "right" vs "wrong," and other phrases that put people on the spot. Instead, point to what you noticed precisely and make it impersonal ("it seems") to avoid confrontation or hurting someone's feelings.
I think this won't work.
🤐
❤️
You should stick to a case. Why no semi-colons?
function get_book_title(language, edition) {
const Title = 'Eloquent'
const editionLabel = 'edition'
return Title + ' ' + language + ': ' + edition + ' ' + editionLabel;
}
I've noticed that using the same case as well as semi-colons throughout a codebase helps with the readability of my code. How about using camelCase for function and variable names?
Avoid blaming the author ("you should") when you see bad code. Instead, provide the reason why changing the code would be beneficial, preferrably drawn from your personal experience.
✋
The name of the function should have get.
function bookTitle() {
return 'Eloquent JavaScript';
}
It looks like the name of this function does not respect the naming convention used in the repository. I would suggest naming it `getBookTitle`.
```suggestion
function getBookTitle() {
```
Provide an alternative. GitHub and GitLab let reviewers add code suggestions that can be committed straight from the pull request.
🗣️
That's not how you do template literals.
function getBookTitle(language, edition) {
return `Eloquent {language}: {edition} edition`;
}
I noticed that you use a template literal (on line 2). However, I suspect after re-reading the documentation at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals that you might be missing the `$` before the curly braces.
Providing links to the Mozilla Developer Network documentation or reliable Stack Overflow questions, for example, can help provide more context about the issue and can be a great learning experience.
🔍
const ELOQUENT = 'Eloquent Python: Twenty-third edition';
function book() {
return ELOQUENT;
}
Hey! I have some thoughts about how we could refactor this function. I sent you a message on Slack. Let's put a video call on our calendars.
Instead of diving in a long conversation, offer to chat on Slack or to hop on a video call. It can get a little frustrating when an exchange keeps going on and on without a resolution. It can also create misunderstandings or give the impression that something is more important than it seems. Talking it out outside of the code review can help with getting on the same page.
✍️
The function needs to return a string.
function getBookTitle(language, edition) {
const bookTitle = {
title: `Eloquent ${language}`,
edition: `${edition} edition`
}
return bookTitle
}
It seems like returning a string might be more handy. Is there a reason why you chose to return an object instead?
You might have strong opinions about how to write code. The author of the pull request might as well. In doubt, ask the author why they wrote the code this way. They might not have thought about the shortcoming of their approach or they might have some interesting suggestions on how to resolve some problems.
🤔
⛰️
Use const, not let.
function getBookTitle(language, edition) {
let languageText = `Eloquent ${language}`;
let editionText = `${edition} edition`;
return `${languageText} ${editionText}`;
}
I noticed that you used `let` in the function. Would it work for you to replace it with `const`? I know, it's so easy to forget! I suspect that you'll get used to it in no time.
It might be a little discouraging when there is a lot of issues with a pull request or when the same mistakes are repeated. Show a little empathy and tell the author it's not a big deal.
🆘
AFAIK LGTM but needs TLC IMO
function getBookTitle(language, edition) {
return `Eloquent ${language}: ${edition} edition`;
}
Nice work! Let me know if you need a hand with some of my comments.
Refrain from using acronyms. You never know if the author of the pull request is familiar with the ones you use. Context might not always be enough to guess what you really mean. At best, they look the acronym up. At worst, they assume you mean something else and misunderstand your feedback.
🙉
+1
function getBookTitle(language, edition) {
return `Eloquent ${language}: ${edition} edition`;
}
Looks good! 👍
Emojis can make your feedback a little more pleasant. Don't hesitate to add a smiley face or a thumb up to give it a warmer touch or illustrate your intention.
🚀
💔
What else?
📚
Thank you!