Slide 1

Slide 1 text

Don't
 Repeat
 Your Code Review Suggestions Soutaro Matsumoto
 @soutaro Ruby X Elixir Conf Taiwan 2018

Slide 2

Slide 2 text

@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

Slide 3

Slide 3 text

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

Slide 4

Slide 4 text

Code Review Example

Slide 5

Slide 5 text

Code Review Example

Slide 6

Slide 6 text

Code Review Example

Slide 7

Slide 7 text

Code Review Automation • LINT tools, like RuboCop, can make some review suggestions • We can automate some of the reviewing process using the tools

Slide 8

Slide 8 text

Code Review Automation Lint/InheritException: EnforcedStyle: standard_error

Slide 9

Slide 9 text

Code Review Automation

Slide 10

Slide 10 text

Code Review Automation

Slide 11

Slide 11 text

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?

Slide 12

Slide 12 text

Let's make a RuboCop plugin!

Slide 13

Slide 13 text

Let's make a RuboCop plugin!

Slide 14

Slide 14 text

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)

Slide 15

Slide 15 text

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?

Slide 16

Slide 16 text

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

Slide 17

Slide 17 text

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

Slide 18

Slide 18 text

• 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

Slide 19

Slide 19 text

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

Slide 20

Slide 20 text

$ 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

Slide 21

Slide 21 text

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

Slide 22

Slide 22 text

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

Slide 23

Slide 23 text

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

Slide 24

Slide 24 text

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

Slide 25

Slide 25 text

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?

Slide 26

Slide 26 text

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)"

Slide 27

Slide 27 text

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

Slide 28

Slide 28 text

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

Slide 29

Slide 29 text

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"

Slide 30

Slide 30 text

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

Slide 31

Slide 31 text

Experiments • With 5 developers using SideCI • Our config contains 57 rules • We casually add Querly rules to share knowledge about our project

Slide 32

Slide 32 text

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

Slide 33

Slide 33 text

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