Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Don't Repeat Your Code Review Suggestions

Don't Repeat Your Code Review Suggestions

Ruby X Elixir Conf Taiwan 2018

1fab9d01b25e99522f3dfd01e3d4cb51?s=128

Soutaro Matsumoto

April 27, 2018
Tweet

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