Code Review Skills For Pythonistas - DjangoCon 2018

As teams and projects grow, code review becomes increasingly important to support the maintainability of complex codebases. In this talk, I’ll cover guidelines for writing consistent python code beyond pep8, how to look out for common python gotchas, and what python tools are available to automate various parts of the review process. Most importantly, I’ll cover the human aspect of code reviews - how we can be better at approaching reviews with empathy and understanding from the perspective of both a reviewer and a submitter. Following these successful code review practices will lead to happier teams and healthier code bases.

This talk is useful for python developers with any amount of experience. No prerequisite knowledge is necessary.

- For those who are just starting out, it will be a great general overview.
- Intermediate developers may not know about the wide variety of tooling that’s available.
- Advanced developers will learn techniques for performing code reviews with empathy.

This talk will enable you to have better code reviews on your teams at work, and a better approach to code reviews in open source projects. You’ll leave with 3 main takeaways:

1. Code Reviews are most effective when conducted with empathy. If you do reviews with growth and learning in mind, they become tools for sharing knowledge instead of an opportunity to bruise egos or show off esoteric knowledge.
2. Python has powerful tooling available for code reviews such as pep8 as a style guide, pylint as a linter, coverage.py to identify test coverage, and vulture to identify dead code.
3. That python style guides beyond pep8 have clear benefits in terms of producing more consistent code that’s easier to review and easier to maintain.

Transcript

1. NINA ZAKHARENKO CODE REVIEW SKILLS FOR PYTHONISTAS @NNJA bit.ly/ djangocodereviews
2. LIVETWEET USE #DJANGOCON @NNJA
3. WHAT WE'LL LEARN TODAY [1/2] > What are the proven benefits of review? > Setting standards > Time saving tools and automation options for Python @nnja
4. WHAT WE'LL LEARN TODAY [2/2] > How to effectively review code > How to submit PRs for maximum impact > Use code review to build a stronger team @nnja
5. WHAT WILL YOU TAKE AWAY FROM THIS TALK? > Novice - Comprehensive overview of code review best practices > Intermediate - Tooling & automation > Advanced - Hardest part – the people factor @nnja
6. NOT ONE SIZE FITS ALL! > team size > 2 vs 10 > product type > agency vs in-house > open source @nnja
7. NOT ONE SIZE FITS ALL! > defect tolerance > high tolerance > phone games > low tolerance > medical equipment > airplanes @nnja
8. CODE REVIEWS CAN BE FRUSTRATING @nnja
9. APPARENT CODE REVIEW FRUSTRATIONS > Adds time demand > Adds process > Can bring up team tensions > “smart” devs think they don’t need it ! @nnja
10. CODE REVIEW BENEFITS @nnja
11. FIND BUGS & DESIGN FLAWS > Design flaws & bugs can be identified and remedied before the code is complete > Case Studies on Review5: > ‑ bug rate by 80% > ‐ productivity by 15% 5 blog.codinghorror.com/code-reviews-just-do-it/ @nnja
12. THE GOAL IS TO FIND BUGS BEFORE YOUR CUSTOMERS DO @nnja
13. SHARED OWNERSHIP & KNOWLEDGE > We’re in this together > No developer is the only expert @nnja
14. LOTTERY FACTOR New Yorker: Can Andy Byford Save the Subways?
15. CODE REVIEW BENEFITS? > Find Bugs > Shared Ownership > Shared Knowledge > Reduce "Lottery Factor" @nnja
16. ENCOURAGE CONSISTENCY > Your code isn’t yours, it belongs to your company > Code should fit your company’s expectations and style (not your own) > Reviews should encourage consistency for code longevity @nnja
17. CODE REVIEWS NEED TO BE UNIVERSAL & FOLLOW GUIDELINES > Doesn’t matter how senior / junior you are > Only senior devs reviewing == bottleneck > Inequality breeds dissatisfaction @nnja
18. HOW? @nnja
19. STYLE GUIDE > Distinguishes personal taste from opinion > Goes beyond PEP8 > Should be agreed upon beforehand > Example: Google's pyguide.md @nnja
20. FORMATTERS - autopep8 - Black - YAPF
21. ! BLACK DEMO @: black.now.sh github.com/jpadilla/black-online
22. VS Code > settings: > "editor.formatOnSave": true > "python.formatting.provider": "black" > "python.formatting.blackArgs": ["--line- length", "100"] > VS Code Black support docs @nnja
23. CONSISTENT CODE IS EASIER TO MAINTAIN BY A TEAM @nnja
24. CODE REVIEW SHOULD BE DONE BY YOUR PEERS & NOT MANAGEMENT @nnja
25. DON’T POINT FINGERS! @nnja
26. WHEN CODE REVIEWS ARE POSITIVE, DEVELOPERS DON’T EXPECT THEIR CHANGES TO BE REVIEWED, THEY WANT THEIR CHANGES TO BE REVIEWED. @nnja
27. LET'S REVIEW: CODE REVIEW FUNDAMENTALS > Reviews done by peers > Style guides & formatters for consistency > No blame culture @nnja
28. HOW SHOULD WE CODE REVIEW? @nnja
29. BE A GREAT SUBMITTER @nnja
31. DON’T GET RUBBER-STAMPED. @nnja
32. DON’T BE CLEVER. READABILITY COUNTS! @nnja
33. GOOD CODE IS LIKE A GOOD JOKE. IT NEEDS NO EXPLANATION. - RUSS OLSEN @nnja
34. STAGES OF REVIEW > 0: before PR submission > 1: PR submitted > 2: (optional) work in progress PR (30-50%) > 3: review almost done (90-100%) > 4: review completed @nnja
35. STAGE 0: BEFORE SUBMISSION @nnja
36. PROVIDE CONTEXT (THE WHY) > What was the motivation for submitting this code? > Link to the underlying ticket/issue > Document why the change was needed > For larger PRs, provide a changelog > Point out any side-effects @nnja
37. YOU ARE THE PRIMARY REVIEWER > Review your own code as if it belonged to someone else. > Anticipate problem areas. @nnja
38. THE PRIMARY REVIEWER > Make sure your code works, and is thoroughly tested. > Don’t rely on others to catch your mistakes. @nnja
39. BEFORE SUBMITTING, TRY A CHECKLIST @nnja
40. ☑ SMALL STUFF > Did you check for reusable code or utility methods? > Did I remove debugger statements? > Are there clear commit messages? @nnja
41. ☑ BIG STUFF > Is my code secure? > Will it scale? > Is it maintainable? > Is it resilient against outages? Tip: Read The Checklist Manifesto @nnja
42. STAGE 1: SUBMITTED @nnja
43. YOU’RE STARTING A CONVERSATION > Don’t get too attached to your code before the review process > Anticipate comments and feedback > Acknowledge you will make mistakes @nnja
44. STAGE 2: (OPTIONAL) WORK IN PROGRESS @nnja
45. SUBMIT WORK IN PROGRESS PULL REQUESTS > Open them your code is 30-50% done > Good idea for bigger features > Don’t be afraid of showing incomplete, incremental work @nnja
46. WHEN CODE IS WORK IN PROGRESS > Feedback to expect: > Architectural issues > Problems with overall design > Design pattern suggestions @nnja
47. STAGE 3: ALMOST DONE @nnja
48. WHEN CODE IS ALMOST DONE > Feedback to expect: > Nit Picks > Variable Names > Documentation & Comments > Small Optimizations @nnja
49. ONE PR PER ISSUE > Solving multiple problems? Break them up into multiple PRs for ease of review. > Solved an unrelated problem? Make a new PR with a separate diff @nnja
50. PREVENT REVIEWER BURNOUT > Reviews lose value when they’re more than 500 lines of code1 > Keep them small & relevant > If a big PR is unavoidable, give the reviewer extra time 1 https://www.microsoft.com/en-us/research/wp-content/uploads/2016/02/ bosu2015useful.pdf @nnja
51. ✔ CHECK CODE WITH AUTOMATED TOOLS @nnja
52. ✔ LINTER @nnja
53. > Coding standard > Error detection > Refactoring help > IDE & editor integration @nnja
54. ! PYLINT RULE: trailing-comma-tuple foo = (1, 3, 4, ) bar = 2, # bar is now a tuple @nnja
55. USE VULTURE.PY TO FIND DEAD OR UNREACHABLE CODE $ pip install vulture $ vulture script.py package/ or $ python -m vulture script.py package/ github.com/jendrikseipp/vulture
56. Sample code def foo(): print("foo") def bar(): print("bar") def baz(): print("baz") foo() bar() Run vulture.py › python -m vulture foo.py foo.py:7: unused function 'baz' (60% confidence) @nnja
57. GIT PRE-COMMIT HOOKS > run linter > check syntax > check for TODOs, debugger statements, unused imports > enforce styling (autopep8, black formatter, sort imports, etc) > option to reject commit if conditions aren't met @nnja
58. pre-commit.com @nnja
59. pre-commit.com > autopep8-wrapper - Runs autopep8 over source > flake8 and pyflakes - Run flake8 or pyflakes on source > check-ast - Check whether files contain valid python > debug-statements - Check for debugger imports and breakpoint() calls @nnja
60. ✔ TESTS > Write them! > Don’t know code health if tests are failing > Tests identify problems immediately @nnja
61. @nnja
62. ✔ CONTINUOUS INTEGRATION CPYTHON USES Azure DevOps Pipelines @nnja
63. ✔ coverage.py % OF CODE EXECUTED WHEN RUNNING A TEST SUITE @nnja
64. ✔ COVERAGE > Coverage tools integrate into GitHub > coverage.py > coveralls.io @nnja
65. STAGE 4: REVIEW COMPLETE @nnja
66. BE RESPONSIVE > Respond to comments if you don't agree with them. > If you won’t implement the suggestion, come to a mutual understanding with the reviewer @nnja
67. IT’S STILL A CONVERSATION If there were comments, let your reviewer know when you’ve pushed changes and are ready to be re- reviewed. @nnja
68. DON’T BIKE-SHED > back & forth > 3 times? step away from the keyboard > talk instead! > record the results of the conversation in the PR > bikeshed.com @nnja
69. VS CODE LIVE SHARE Download Extension
70. FIGHT FOR WHAT YOU BELIEVE, BUT GRACEFULLY ACCEPT DEFEAT. @nnja
71. DON’T TAKE FEEDBACK PERSONALLY. IT’S AN OPPORTUNITY FOR GROWTH. @nnja
72. HOW TO BE A GREAT SUBMITTER? > Provide the why (context!) > Review your own code > Expect conversation > Submit in progress work @nnja
73. HOW TO BE A GREAT SUBMITTER? > Use automated tools > Be responsive > Accept defeat @nnja
74. #1: BE A GREAT REVIEWER @nnja
75. @nnja
76. BE OBJECTIVE “THIS METHOD IS MISSING A DOCSTRING” INSTEAD OF “YOU FORGOT TO WRITE A DOCSTRING” @nnja
77. ASK QUESTIONS DON’T GIVE ANSWERS > “Would it make more sense if... ?” > “Did you think about... ? ” @nnja
78. OFFER SUGGESTIONS > “It might be easier to...” > “We tend to do it this way...” @nnja
79. AVOID THESE TERMS > Simply > Easily > Just > Obviously > Well, actually... Recurse Center Social Rules
80. ... NOW, SIMPLY @nnja
81. HAVE CLEAR FEEDBACK > Strongly support your opinions > Share How & Why > Link to supporting docs, stackoverflow, etc @nnja
82. THIS IS NOT CLEAR FEEDBACK @nnja
83. COMPLIMENT GOOD WORK & GREAT IDEAS @nnja
84. DON'T BE A PERFECTIONIST @nnja
85. DON’T BE A PERFECTIONIST > Don’t let perfect get in the way of perfectly acceptable. > Prioritize what’s important to you. > Usually 90% there is good enough. @nnja
86. IT’S OK TO NIT-PICK > Syntax Issues > Spelling Errors > Poor Variable Names > Missing corner-cases > Specify: Are your nitpicks blocking merge? Save the nit-picks for last, after any pressing architecture, design, or other large scale issues have been addressed. @nnja
87. Don't burn out. Studies show reviewer should look at 200-400 lines of code at a time for maximum impact2. 2 https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/
88. Limit reviews to 400 lines in 60 mins to maximize effectiveness3. 3 https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/
89. TRY TO DO REVIEWS IN 24-48 HOURS
90. HOW CAN WE BE A GREAT REVIEWER? > Have Empathy > Watch your Language > Have Clear Feedback > Give Compliments @nnja
91. HOW CAN WE BE A GREAT REVIEWER? > Don’t be a perfectionist > Avoid Burn Out > Complete in 24-48 hours @nnja
92. I'VE BECOME A MUCH BETTER PROGRAMMER BY PARTICIPATING IN CODE REVIEWS @nnja
93. CODE REVIEWS BUILD A STRONGER TEAM @nnja
94. HIRING SENIOR ENGINEERS IS HARD. YOU CAN HIRE JUNIOR ENGINEERS, AND GROW THEM INTO FUNCTIONAL PRODUCTIVE PARTS OF YOUR TEAM. - SASHA LAUNDY @nnja
95. IF YOU’RE NOT DOING CODE REVIEWS, YOU’RE MISSING A BIG OPPORTUNITY. @nnja
96. REMEMBER... > Allocate the time > Develop, don’t force the process > Not one size fits all > Or a one stop fix > Use in addition to tests, QA, etc for maximum impact @nnja
97. WHAT DID WE LEARN? @nnja
98. @nnja
99. REVIEWS DECREASE WTFS/M BY INCREASING CODE QUALITY LONG TERM @nnja
100. LESS WTFS ➡ HAPPIER DEVS! @nnja
101. SLIDES: bit.ly/djangocodereviews PYTHON @ MICROSOFT bit.ly/pythonazure (ADDITIONAL READING ON NEXT SLIDE) @nnja
102. RESOURCES & ADDITIONAL READING > Top 3 Django Gotchas to Catch during Code Review > Microsoft Study on Effective Code Review > Code Reviews: Just do it > Code Project - Code Review Guidelines > Great Example Checklist > Best Practices for Code Review > Rebecca's Rules for Constructive Code Review > My Big Fat Scary Pull Request > The Gentle Art of Patch Review - Sage Sharp > Watch: Raymond Hettinger - Beyond PEP8 @nnja
103. EXAMPLE STYLE GUIDES > Python > Plone Google has many good, but strict style guides at: https://github.com/google/styleguide Doesn't matter which one you use. Pick one and stick with it. @nnja