1 of 56

Code review tips for Pythonistas

2019/08/17 | Masataka Arai

2 of 56

Who am I?

  • Masataka Arai @massa142

​

  • SQUEEZE Inc.

​

  • PyCon JP Staff

​

  • DjangoCongress JP Staff

​

  • Python Boot Camp Instructor

3 of 56

PyCon Korea for the first time in 3 years

4 of 56

Are you writing codes within a team? 🙋

5 of 56

Are you doing code reviews? 🙋

6 of 56

​

Code review is an essential part of team development, but surprisingly difficult and can be frustrating 😵

​

7 of 56

In this talk, I'll introduce code review tips that you can use from today 👍

​

And these tips are usually practiced in SQUEEZE Inc.

​

8 of 56

Agenda

  • Style Guide / Formatter
  • How to write Pull Requests
  • Checking Pull Request size
  • How to write review comments
  • What to do if a long discussion occurs
  • Mob code review
  • Playful comments

9 of 56

Style Guide / Formatter

10 of 56

Common story 1

  • “Please use single quotes.”
  • “Please arrange the import order.”
  • “It'll be easier to read if there is a line break here.”

​

​

11 of 56

Style Guide

​

​

  • can prevent war from each taste.
  • can get code consistency.
  • Python has PEP8 as a style guide, so we can think based on that.
  • Examples: Google Python Style Guide, Plone style guides

​

12 of 56

Style Guide

​

🚨 Warning

  • shouldn't create a rule that cannot be detected by lint tools or formatters.

​

Example: In assertEqual (A, B), you must give the expected value to A and the actual value to B.

=> But either is OK in Python unittest.

​

13 of 56

Style Guide

  • Rules that cannot be detected automatically are not good 🙅
    • Because you need to check those with your own eyes.

​

​

14 of 56

Formatter

Tools that automatically format codes.

​

  • autopep8
  • Black
  • YAPF

​

15 of 56

Formatter

​

  • Black is new and has many editor integrations, so popular recently.
  • Each one has its own features, so you can choose according to your taste.
  • It convenient to run formatter in git pre-commit hook.

​

16 of 56

  • 🤖 The machine does lint / auto-formatting by a style guide.
  • 😎 We do other essential code reviews.

​

​

This division of labor is important.

We have limited time to use, so let's automate if possible!

​

17 of 56

How to write Pull Requests

18 of 56

Common story 2

​

​

  • It's hard to review because you don’t know the background and purpose of the fix 😇
  • If you don't run a server locally, you can't understand some UI changes 🙄

​

19 of 56

Pull Request rules

​

  • PULL_REQUEST_TEMPLATE.md
  • can get the minimum information required for review by providing a format.

​

20 of 56

Pull Request template in SQUEEZE Inc.

21 of 56

Pull Request rules

​

  • ticket links (JIRA, GitHub Issue, Sentry, etc.)
  • description
  • checked list which you tested to confirm behaviors
  • related Pull Requests
  • screenshots or GIF animations to confirm the UI changes
  • whether there are migration files
  • whether there are changes in the cron settings
  • whether there are new or updated dependencies

​

22 of 56

screenshots or GIF animations

23 of 56

Checking Pull Request size

24 of 56

Common story 3

​

​

  • “OMG! There are so many diffs. Review is impossible.”
  • “Thank you for submitting PR, but it might be better to think from the design.”

25 of 56

Common story 3

https://twitter.com/iamdevloper/status/397664295875805184

26 of 56

Split Pull Request into small pieces

​

  • We may overlook bugs if there're so many points to review 🐛
  • Responsibility is clear, so small Pull Request makes review easy.

​

​

27 of 56

​

Checking diff size by Danger

​

  • Danger: runs during your CI process, and leaves messages inside your Pull Requests based on rules.

​

​

28 of 56

dangerfile.js

  1. // Warn when PR size is large
  2. const bigPRThreshold = 350;
  3. if (danger.github.pr.additions + danger.github.pr.deletions > bigPRThreshold) {
  4. warn(':exclamation: Big PR');
  5. markdown('> Pull Request size seems relatively large. If Pull Request contains multiple changes, split each into separate PR will helps faster, easier review.');
  6. }

29 of 56

Sharing WIP

​

  • Work In Progress
  • leads to an open work process
  • Before implementing everything, create a WIP Pull Request.
  • You can get advices about design or policy at the beginning.

​

​

30 of 56

GitHub provides draft Pull Request

31 of 56

How to write review comments

32 of 56

Common story 4

​

  • “It seems better to xxx here.”
    • I don't know if this correction needs to be done or it is left to my judgement 🤔
  • Text-based communication is difficult (especially in Japanese 😛)

33 of 56

Comment prefix

​

  • must
  • IMO (In my opinion)
  • nits
  • ask

Example case:

When a hotfix release, you can decide to fix only the [must] part now and fix [nits] and [IMO] later.

​

​

34 of 56

※ Sorry for Japanese comments

35 of 56

※ Sorry for Japanese comments

36 of 56

Question using [ask] prefix

​

  • Answering questions can lead to finding bugs or sharing knowledge 💡
  • The [ask] prefix makes it easy to ask questions 💬

​

​

37 of 56

What to do if a long discussion occurs

38 of 56

Common story 5

​

​

  • The review causes hot discussions sometimes 🔥
  • Only the comments increase and we can't merge pull request 🚫

​

39 of 56

Let's talk face-to-face

​

  • Text-based communication is difficult.
  • If you think it's faster to talk, let’s talk while looking at the code.
    • Zoom or Slack Call, Visual Studio Live Share (if you’re not in the same space)

​

​

40 of 56

Move to Issue

​

  • If you have a long discussion but this topic isn't urgent, you should let move to Issue 📦
    • “Since it has become long, let's continue with Issue. Once in this Pull Request, I'll use idea A instead of idea B.”

​

​

​

41 of 56

Mob code review

42 of 56

Common story 6

​

  • “But I need to work on my tickets, so I'll do code reviews later.”
  • Unreviewed Pull Requests continue to increase 📈

​

43 of 56

Mob code review

​

  • Let's review codes like mob programming

> Mob programming is a software development approach where the whole team works on the same thing, at the same time, in the same space, and at the same computer.

​

​

​

​

https://en.wikipedia.org/wiki/Mob_programming

​

​

​

44 of 56

Mob code review

​

  • avoids a bad practice that only one senior developer can review the code.
  • allows you to quickly ask the authors.
  • leads to knowing what other people are thinking in code review.

​

​

​

45 of 56

Mob code review

46 of 56

Playful comments

47 of 56

Common story 7

​

​

  • Emotions cannot be seen with letters alone 😑
    • “looks fine.”
    • ”plz fix this.”
  • It can be misunderstood as "he/she is angry." 😠

​

48 of 56

Playful comments

​

  • LGTM image
  • emoji
  • exclamation mark "!!!"

Let's express your emotions by using these!!!

​

​

​

49 of 56

LGTM image

50 of 56

LGTM image

51 of 56

emoji

52 of 56

Summary

53 of 56

Summary

​

  • leave tasks that can be checked and fixed automatically to the machine.
  • send Pull Requests so that reviewers can easily review.
  • write review comments clearly so that there is no misunderstanding.

=> Let's work on essential reviews with ❤️ for team members!!!

​

​

​

​

54 of 56

Summary

Through better code reviews,

Let's practice Readability counts.

​

55 of 56

Thanks :)

Here are my SNS accounts.

​

​

56 of 56

PyCon Korea 2019

제목 텍스트