mrwinton/better-best-practices.md Secret

## better-best-practices.md

      
    Raw
  

              better-best-practices.md
            
          
    Better Best Practices [Working Title]

Have you ever received a code review that went something along the lines of:

There are a few places that I've commented which don't follow the best
practices documented here, let's fix up before proceeding!

Engineering teams commonly align on practices that we believe encourage good
outcomes in a best-practices.md document or similar. These documents are the
melting pot in which company history, community standards, new initiatives and
hard-earned bug fix lessons meet. So it is only natural for reviews like the one
above to occur for changes to be consistent with the
best-practices.md.
All good, right? Yes, but no.
I believe there are a couple of points of friction with this process.
First, the process inherently relies on the team's ability to recall and
evaluate every practice, in every change, in every code review, in the earnest
hope that bad practices don't slip through.
Second, the process can dilute the signal to noise ratio of the code review,
wherein the worst-case scenario, reviewers and reviewees miss opportunities for
discussion on nuances of the change itself.
So at Velory, we wondered how we could improve this process. Making it easier to
apply our best practices while tightening the feedback loop so that issues are
caught long before code review.
Imagine the nirvana when you are writing code that would introduce a bad practice
and your editor gave the instant feedback, "Hi, avoid Foo. Use Bar instead", that
would be delightful. That's what we're after.
Towards nirvana

We use Rails to develop our applications and RuboCop to lint our Ruby code.
RuboCop does a marvellous job of checking and maintaining code style through
Cops — rules that identify issues, and optionally automatically fix them too.
At Velory, our first step on this journey has been through leveraging RuboCop. By
synthesising our best practices into Cops we are essentially able to lint our
best practices — consistently✅ and with a short feedback loop✅!
A reasonable workflow for turning a practice into a Cop has been to:


Choose a best practice
Not all best practices are equal, but a good place to start are best
practices that are grep(1)-able. In this example, we can use:
  - Avoid `let` and `let!` in specs, prefer four-phase test style
Which is grep(1)-able with:
  grep 'let(.*)' -r spec/**/*.rb


Identify examples
Often there are a few ways in which a bad practice can be written, so
identifying these ways helps towards writing a water-tight Cop.
  # bad
  let(:foo) { "foo" }

  # bad, bad
  let!(:bar) { "bar" }


Write failing specs
Using the examples we can now write failing specs that will guide the
implementation.
  require "spec_helper"
  require "rubocop"
  require "rubocop/rspec/support"
  require "rubocop/cop/velory/lets_not"

  RSpec.describe RuboCop::Cop::Velory::LetsNot do
    include RuboCop::RSpec::ExpectOffense

    it "adds offenses for uses of `let`" do
      expect_offense(<<~RUBY)
        let(:foo) { "foo" }
        ^^^^^^^^^ Avoid `let` and `let!`. Instead inline the 
                  instance within the `it` block to follow the 
                  four-phase test pattern.
      RUBY
    end

    it "adds offenses for uses of `let!`" do
      expect_offense(<<~RUBY)
        let!(:bar) { "bar" }
        ^^^^^^^^^^ Avoid `let` and `let!`. Instead inline the 
                   instance within the `it` block to follow the 
                   four-phase test pattern.
      RUBY
    end

    def cop
      @_cop ||= RuboCop::Cop::Velory::LetsNot.new
    end
  end
This is in my opinion the most important step. Here we design our developer
experience, and to make it a delightful one, I recommend using an actionable
message. So the developer understands the why of the offense and is
equipped with the context to successfully move forwards with a solution.


Write the Cop
This is the "Draw the rest of the Owl" step, where we need to be familiar
with how RuboCop matches code.
A tip for writing Cops where the example code is grep(1)-able is to start with
the on_send matcher and node instance methods to find matches. This
approach takes us pretty far and is all we require for our "Let's Not" best
practice.
  require "rubocop"

  class RuboCop::Cop::Velory::LetsNot < RuboCop::Cop::Base
    MSG =
      "Avoid `let` and `let!`. Instead inline the instance " \
      "within the `it` block to follow the four-phase test " \ 
      "pattern."

    def on_send(node)
      return unless node.command?(:let) || node.command?(:let!)

      add_offense(node)
    end
  end


Run on the project
Once the specs are green it's time to run the Cop on the project!
  ❯ bundle exec rubocop
  Inspecting 1155 files
  ......................CC...........C.............C..............
  
  1155 files inspected, 43 offenses detected
With RuboCop linting our "Let's Not" best practice we find fourty-three
existing offenses in one of our projects. Finding existing offenses to the
rule is not an entirely surprising result, c'est la vie. However, on closer
inspection, all of these offenses are used within shared_examples.
In scenarios like this we have an excellent opportunity to evaluate the best
practice as a team, questioning:

is this an exception to the best practice?
is it really a best practice if we have gone against it X times?
should we write out the existing offenses?

If the team thinks it's an exception, then we can loop back to step three,
refine our examples and update the Cop with the exception in mind. If the
team thinks it's no longer a best practice, we can delete it. And if the team
thinks that we should write out the offenses we can use
rubocop --auto-gen-config and leverage shitlist driven development.


Parting Thoughts

Encoding your best-practices.md document with RuboCop is not a drop-in
replacement, but it can be an excellent tool in your team's toolbox for
tightening these feedback loops and ensuring that best practices are held.
Writing custom Cops is not always straightforward and some best practices are
too subjective to be converted. But that's OK. We can delegate our objective
best practices to RuboCop and save our thinking caps for the things that matter.
For further reading I recommend RuboCop's custom Cop development guide,
EvilMartian's blog post on custom Cops, as well as the open sourced custom
Cops from Airbnb, Discourse, GitHub, GitLab and Shopify.
Finally, here is our finished "Let's Not" best practice which has been refined
to "Let's Not (Outside Of Shared Examples)":
  require "rubocop"

  class RuboCop::Cop::Velory::LetsNot < RuboCop::Cop::Base
    MSG =
      "Avoid `let` and `let!`. Instead inline the instance within " \
      "the `it` block to follow the four-phase test pattern. " \
      "Shared examples are an exception to this rule."

    def_node_matcher :in_shared_example?, <<-PATTERN
      {
        (block (send _ #shared_example_context? ...) ...)
      }
    PATTERN

    def on_send(node)
      return unless let_used?(node) && !in_shared_example_block?(node)

      add_offense(node)
    end

    private

    def let_used?(node)
      node.command?(:let) || node.command?(:let!)
    end

    def in_shared_example_block?(node)
      node.each_ancestor(:block).any?(&method(:in_shared_example?))
    end

    def shared_example_context?(element)
      %w[
         it_behaves_like 
         it_should_behave_like 
         include_examples 
         shared_examples
       ].include?(element.to_s)
    end
  end
And with that, bliss:
  ❯ bundle exec rubocop
  Inspecting 1155 files
  ................................................................

  1155 files inspected, no offenses detected