Don't Repeat Your Code Review Suggestions

Ruby X Elixir Conf Taiwan 2018

Transcript

1. Don't
 Repeat
 Your Code Review Suggestions Soutaro Matsumoto
 @soutaro Ruby X Elixir Conf Taiwan 2018
2. @soutaro • CTO at SideCI, Inc. • Interested in program analysis • Develops a type checking tool for Ruby called Steep • Will have a talk at RubyKaigi 2018
3. Code Review • Give feedback to code • To improve code structure and quality • To find bugs • To fix styling issues • Share knowledge about frameworks/projects • Using tools like Pull Request in GitHub
4. Code Review Example
5. Code Review Example
6. Code Review Example
7. Code Review Automation • LINT tools, like RuboCop, can make some review suggestions • We can automate some of the reviewing process using the tools
8. Code Review Automation Lint/InheritException: EnforcedStyle: standard_error
9. Code Review Automation
10. Code Review Automation
11. Project Specific Rules • General tools do not support them • Rules about APIs defined in the project • RuboCop cannot know about #with_octokit • Does not match/maybe conflicting with general best practices • Does anyone have .env.sample in your repo?
12. Let's make a RuboCop plugin!
13. Let's make a RuboCop plugin!
14. RuboCop Plugin • RuboCop provides plugin mechanism for custom rules • You can write arbitrary Ruby code as a plugin • We rarely develop the plugin for project specific rules • It needs a lot of work (high cost), but the plugin is only for a few people (low return)
15. RuboCop Plugin • RuboCop provides plugin mechanism for custom rules • You can write arbitrary Ruby code as a plugin • We rarely develop the plugin for project specific rules • It needs a lot of work (high cost), but the plugin is only for a few people (low return) What if we can make the development cost low enough?
16. Requirements • Encourages users to add rules • Writing rules • Reading rules • Testing rules • Rules to find out method calls and print messages to show possible improvements
17. Key Idea • Define a declarative language to define rules • A restricted language to define patterns of code detect • The language will be much easier to write/read than Ruby • Users will define their own rules using the language
18. • When the program refers ENV constant, print the message rules: - id: com.example.env_sample pattern: ENV message: > When you use env var, update .env.sample for other developers
19. rules: - id: com.example.octokit pattern: octokit() message: > To make a request to GitHub API, use #with_octokit to handle failures • When the program calls octokit method, print the message
20. $ gem install querly $ querly init $ querly check http://github.com/soutaro/querly $ querly check lib/main.rb:1:14 Exception You probably should use StandardError lib/main.rb:12:5 File.open("foo.txt") Use block to read/write file spec/main_spec.rb:6:4 pp 1+2 Delete debug print
21. Implementation • Pattern matching against Ruby AST • To handle ambiguity of Ruby grammar • Querly pattern parsing is done by a LALR(1) parser implemented using Racc Ruby source AST Querly configuration Pattern Pattern matching Output
22. Patterns self.pp(...) pp user: user, organization: organization string(:symbol:, !null: _) create_table(:users) do |t| t.string :email t.string :name, null: false end save!() [conditional] if user.save! render :show else render :error, locals: { user: user } end
23. Design Decisions • How flexible the patterns should be • Ruby DSL or different format • Docs and tests • False detections Principle: encourage users to write their own rules
24. Limited Flexibility • Flexibility is not important • The rules are project-specific ones • The patterns you can define in Querly is restricted to encourage adding new rules • Patterns you cannot write means it is out of the scope of Querly Rule flexibility Definition difficulty Querly RuboCop
25. Rule Definition in YAML • Different from Rakefile/Gemfile • Ruby is too flexible rules: - id: com.sample.non_null_migration pattern: "_.string(:symbol:, !null: _)" message: < Do you really want a nullable column?
26. Docs and Tests • Rule can include justification and examples to explain what the rule is fore more correctly • Examples are executable spec rules: - id: com.sample.non_null_migration pattern: "_.string(:symbol:, !null: _)" message: < Do you really want a nullable column? justification: - When you want a nullable column examples: - before: "t.string(:name)" after: "t.string(:name, null: true)"
27. Allow False Detections • Analysis of Ruby programs is really difficult • If we want more true detections, we have to allow false detections False detections True detections RuboCop Querly Extra layer to handle false detections
28. Detection Filtering • If number of detections you have to review at one time is not too big, false detections are ok • Hook pull requests! • Run Querly analysis when you open new pull request • Ignore issues from the code which is not changed in the pull request
29. Using OSS • Run Querly in CI and post the detected issues as comments to pull requests • Pronto & pronto-querly • $ gem i pronto
 $ gem i pronto-querly • reviewdog runner: querly: cmd: "bundle exec querly check" errorformat: - "%f:%l:%c\t%m"
30. Using SideCI • https://sideci.com • Managed service to run Querly and other LINT tools • Provides issue manager to review detected issues instead of posting comments to GitHub • Allows closing false detected issues Promoted
31. Experiments • With 5 developers using SideCI • Our config contains 57 rules • We casually add Querly rules to share knowledge about our project
32. Finding New Rules • When we are writing a review comment, we always think if we can write a Querly rule for that • When we are deprecating an API: octokit() • We can ship new API without completely replacing old one • When hidden requirements are uncovered:
 validates(:symbol:, ..., uniqueness: _, ...) • Better than asking your teammates to read all of the docs
33. Conclusion • Developed a tool for code review automation which covers project specific rules • Rules are defined by patterns • Used in my team to DRY up code review suggestions • Design decisions • Limited flexibility for ease of definitions • Handle false detections by extra layer to filter them