Giter Club home page Giter Club logo

ruby-style-guide's Introduction

layout title permalink
base
Ruby Style Guide
/

Ruby Style Guide

Ruby is the main language at Shopify. We are primarily a Ruby shop and we are probably one of the largest out there. Ruby is the go-to language for new web projects and scripting.

We expect all developers at Shopify to have at least a passing understanding of Ruby. It's a great language. It will make you a better developer no matter what you work in day to day. What follows is a loose coding style to follow while developing in Ruby.

This Style Guide is the result of over a decade of Ruby development at Shopify. Much of its content is based on Bozhidar Batsov's Ruby Style Guide, adapted to Shopify by many contributors.

Adoption with RuboCop

We recommend using RuboCop in your Ruby projects to help you adopt this Style Guide. To know how to install and use RuboCop please refer to RuboCop's official documentation.

We offer a default RuboCop configuration you can inherit from and be in sync with this Style Guide. To use it, you can add this to your Gemfile:

gem "rubocop-shopify", require: false

And add to the top of your project's RuboCop configuration file:

inherit_gem:
  rubocop-shopify: rubocop.yml

Any Include or Exclude configuration provided will be merged with RuboCop's defaults.

For more information about inheriting configuration from a gem please check RuboCop's documentation.

Table of Contents

General

  • Make all lines of your methods operate on the same level of abstraction. (Single Level of Abstraction Principle)

  • Code in a functional way. Avoid mutation (side effects) when you can.

  • Avoid defensive programming

    Overly defensive programming may safeguard against errors that will never be encountered, thus incurring run-time and maintenance costs.

  • Avoid mutating arguments.

  • Avoid monkeypatching.

  • Avoid long methods.

  • Avoid long parameter lists.

  • Avoid needless metaprogramming.

  • Prefer public_send over send so as not to circumvent private/protected visibility.

  • Write ruby -w safe code.

  • Avoid more than three levels of block nesting.

Layout

  • Use UTF-8 as the source file encoding.

  • Use 2 space indent, no tabs.

  • Use Unix-style line endings.

  • Avoid using ; to separate statements and expressions. Use one expression per line.

  • Use spaces around operators, after commas, colons and semicolons, around { and before }.

  • Avoid spaces after (, [ and before ], ).

  • Avoid space after the ! operator.

  • Avoid space inside range literals.

  • Avoid space around method call operators.

    # bad
    foo . bar
    
    # good
    foo.bar
  • Avoid space in lambda literals.

    # bad
    a = -> (x, y) { x + y }
    
    # good
    a = ->(x, y) { x + y }
  • Indent when as deep as the case line.

  • When assigning the result of a conditional expression to a variable, align its branches with the variable that receives the return value.

    # bad
    result =
      if some_cond
        # ...
        # ...
        calc_something
      else
        calc_something_else
      end
    
    # good
    result = if some_cond
      # ...
      # ...
      calc_something
    else
      calc_something_else
    end
  • When assigning the result of a begin block, align rescue/ensure/end with the start of the line

    # bad
    host = begin
             URI.parse(value).host
           rescue URI::Error
             nil
           end
    
    # good
    host = begin
      URI.parse(value).host
    rescue URI::Error
      nil
    end
  • Use empty lines between method definitions and also to break up methods into logical paragraphs internally.

  • Use spaces around the = operator when assigning default values to method parameters.

  • Avoid line continuation \ where not required.

  • Align the parameters of a method call, if they span more than one line, with one level of indentation relative to the start of the line with the method call.

    # starting point (line is too long)
    def send_mail(source)
      Mailer.deliver(to: "[email protected]", from: "[email protected]", subject: "Important message", body: source.text)
    end
    
    # bad (double indent)
    def send_mail(source)
      Mailer.deliver(
          to: "[email protected]",
          from: "[email protected]",
          subject: "Important message",
          body: source.text)
    end
    
    # good
    def send_mail(source)
      Mailer.deliver(
        to: "[email protected]",
        from: "[email protected]",
        subject: "Important message",
        body: source.text,
      )
    end
  • When chaining methods on multiple lines, indent successive calls by one level of indentation.

    # bad (indented to the previous call)
    User.pluck(:name)
        .sort(&:casecmp)
        .chunk { |n| n[0] }
    
    # good
    User
      .pluck(:name)
      .sort(&:casecmp)
      .chunk { |n| n[0] }
  • Align the elements of array literals spanning multiple lines.

  • Limit lines to 120 characters.

  • Avoid trailing whitespace.

  • Avoid extra whitespace, except for alignment purposes.

  • End each file with a newline.

  • Avoid block comments:

    # bad
    =begin
    comment line
    another comment line
    =end
    
    # good
    # comment line
    # another comment line
  • Place the closing method call brace on the line after the last argument when opening brace is on a separate line from the first argument.

    # bad
    method(
      arg_1,
      arg_2)
    
    # good
    method(
      arg_1,
      arg_2,
    )
  • Place each element/argument on a new line when wrapping a method call, hash, or array on multiple lines.

    # bad
    
    method(arg_1, arg_2,
      arg_3
    )
    
    [
      value_1, value_2,
      value_3,
    ]
    
    {
      key1: value_1,
      key2: value_2, key3: value_3,
    }
    
    # good
    
    method(
      arg_1,
      arg_2,
      arg_3,
    )
    
    [
      value_1,
      value_2,
      value_3,
    ]
    
    {
      key1: value_1,
      key2: value_2,
      key3: value_3,
    }
    
    # good (special cases)
    
    # Single argument method call
    method({
      foo: bar,
    })
    
    # Last argument, itself is multiline
    class User
      after_save :method, if: -> {
        do_some_checks
      }
    end
    
    # Single value array
    errors = [{
      error_code: 1234,
      error_message: "This is an error",
    }]
  • Separate magic comments from code and documentation with a blank line.

    # good
    # frozen_string_literal: true
    
    # Some documentation for Person
    class Person
      # Some code
    end
    
    # bad
    # frozen_string_literal: true
    # Some documentation for Person
    class Person
      # Some code
    end
  • Use empty lines around attribute accessor.

    # bad
    class Foo
      attr_reader :foo
      def foo
        # do something...
      end
    end
    
    # good
    class Foo
      attr_reader :foo
    
      def foo
        # do something...
      end
    end
  • Avoid empty lines around method, class, module, and block bodies.

    # bad
    class Foo
    
      def foo
    
        begin
    
          do_something do
    
            something
    
          end
    
        rescue
    
          something
    
        end
    
        true
    
      end
    
    end
    
    # good
    class Foo
      def foo
        begin
          do_something do
            something
          end
        rescue
          something
        end
      end
    end

Syntax

  • Use :: only to reference constants (this includes classes and modules) and constructors (like Array() or Nokogiri::HTML()). Avoid :: for regular method invocation.

  • Avoid using :: for defining class and modules, or for inheritance, since constant lookup will not search in parent classes/modules.

    # bad
    module A
      FOO = "test"
    end
    
    class A::B
      puts FOO  # this will raise a NameError exception
    end
    
    # good
    module A
      FOO = "test"
    
      class B
        puts FOO
      end
    end
  • Use def with parentheses when there are parameters. Omit the parentheses when the method doesn't accept any parameters.

  • Avoid for.

  • Avoid then.

  • Favour the ternary operator(?:) over if/then/else/end constructs.

    # bad
    result = if some_condition then something else something_else end
    
    # good
    result = some_condition ? something : something_else
  • Use one expression per branch in a ternary operator. This also means that ternary operators must not be nested. Prefer if/else constructs in these cases.

  • Avoid multiline ?: (the ternary operator); use if/unless instead.

  • Use when x then ... for one-line cases.

  • Use ! instead of not.

  • Prefer &&/|| over and/or.

  • Favour unless over if for negative conditions.

  • Avoid unless with else. Rewrite these with the positive case first.

  • Use parentheses around the arguments of method invocations. Omit parentheses when not providing arguments. Also omit parentheses when the invocation is single-line and the method:

    • is a class method call with implicit receiver.
    • is called by syntactic sugar (e.g: 1 + 1 calls the + method, foo[bar] calls the [] method, etc).
    # bad
    class User
      include(Bar)
      has_many(:posts)
    end
    
    # good
    class User
      include Bar
      has_many :posts
      SomeClass.some_method(:foo)
    end
    • is one of the following methods:
      • require
      • require_relative
      • require_dependency
      • yield
      • raise
      • puts
  • Omit the outer braces around an implicit options hash.

  • Use the proc invocation shorthand when the invoked method is the only operation of a block.

    # bad
    names.map { |name| name.upcase }
    
    # good
    names.map(&:upcase)
  • Prefer {...} over do...end for single-line blocks.

  • Prefer do..end over {...} for multi-line blocks.

  • Omit return where possible.

  • Omit self where possible.

    # bad
    self.my_method
    
    # good
    my_method
    
    # also good
    attr_writer :name
    
    def my_method
      self.name = "Rafael" # `self` is needed to reference the attribute writer.
    end
  • Wrap assignment in parentheses when using its return value in a conditional statement.

    if (value = /foo/.match(string))
  • Use ||= to initialize variables only if they're not already initialized.

  • Avoid using ||= to initialize boolean variables.

    # bad - would set enabled to true even if it was false
    @enabled ||= true
    
    # good
    @enabled = true if @enabled.nil?
    
    # also valid - defined? workaround
    @enabled = true unless defined?(@enabled)
  • Avoid spaces between a method name and the opening parenthesis.

  • Prefer the lambda literal syntax over lambda.

    # bad
    l = lambda { |a, b| a + b }
    l.call(1, 2)
    
    l = lambda do |a, b|
      tmp = a * 7
      tmp * b / 50
    end
    
    # good
    l = ->(a, b) { a + b }
    l.call(1, 2)
    
    l = ->(a, b) do
      tmp = a * 7
      tmp * b / 50
    end
  • Prefer proc over Proc.new.

  • Prefix unused block parameters with _. It's also acceptable to use just _.

  • Prefer a guard clause when you can assert invalid data. A guard clause is a conditional statement at the top of a function that bails out as soon as it can.

    # bad
    def compute_thing(thing)
      if thing[:foo]
        update_with_bar(thing)
        if thing[:foo][:bar]
          partial_compute(thing)
        else
          re_compute(thing)
        end
      end
    end
    
    # good
    def compute_thing(thing)
      return unless thing[:foo]
      update_with_bar(thing[:foo])
      return re_compute(thing) unless thing[:foo][:bar]
      partial_compute(thing)
    end
  • Prefer keyword arguments over options hash.

  • Prefer map over collect, find over detect, select over find_all, size over length.

  • Prefer Time over DateTime.

  • Prefer Time.iso8601(foo) instead of Time.parse(foo) when expecting ISO8601 formatted time strings like "2018-03-20T11:16:39-04:00".

  • Avoid returning from a begin block in assignment contexts. If you return from a method inside a begin block, the return will prevent the assignment from taking place, potentially causing confusing memoization bugs.

    # bad
    def foo
      @foo ||= begin
        return 1 if flag?
        2
      end
    end
    
    # good
    def foo
      @foo ||= begin
        if flag?
          1
        else
          2
        end
      end
    end

Naming

  • Use snake_case for symbols, methods, and variables.

  • Use CamelCase for classes and modules, but keep acronyms like HTTP, RFC, XML uppercase.

  • Use snake_case for naming files and directories, e.g. hello_world.rb.

  • Define a single class or module per source file. Name the file name as the class or module, but replacing CamelCase with snake_case.

  • Use SCREAMING_SNAKE_CASE for other constants.

  • When using inject with short blocks, name the arguments according to what is being injected, e.g. |hash, e| (mnemonic: hash, element)

  • When defining binary operators, name the parameter other(<< and [] are exceptions to the rule, since their semantics are different).

  • Name predicate methods with a ?. Predicate methods are methods that return a boolean value.

  • Avoid ending method names with a ? if they don't return a boolean.

  • Avoid prefixing method names with is_.

    # bad
    def is_empty?
    end
    
    # good
    def empty?
    end
  • Avoid starting method names with get_.

  • Avoid ending method names with ! when there is no equivalent method without the bang. Bangs are to mark a more dangerous version of a method, e.g. save returns a boolean in ActiveRecord, whereas save! will throw an exception on failure.

  • Avoid magic numbers. Use a constant and give it a meaningful name.

  • Avoid nomenclature that has (or could be interpreted to have) discriminatory origins.

Comments

  • Include relevant context in comments, as readers might be missing it.

  • Keep comments in sync with code.

  • Write comments using proper capitalization and punctuation.

  • Avoid superfluous comments. Focus on why the code is the way it is if this is not obvious, not how the code works.

Classes and Modules

  • Prefer modules to classes with only class methods. Classes should be used only when it makes sense to create instances out of them.

  • Prefer extend self over module_function.

    # bad
    module SomeModule
      module_function
    
      def some_method
      end
    
      def some_other_method
      end
    end
    
    # good
    module SomeModule
      extend self
    
      def some_method
      end
    
      def some_other_method
      end
    end
  • Use a class << self block over def self. when defining class methods, and group them together within a single block.

    # bad
    class SomeClass
      def self.method1
      end
    
      def method2
      end
    
      private
    
      def method3
      end
    
      def self.method4 # this is actually not private
      end
    end
    
    # good
    class SomeClass
      class << self
        def method1
        end
    
        private
    
        def method4
        end
      end
    
      def method2
      end
    
      private
    
      def method3
      end
    end
  • Respect the Liskov Substitution Principle when designing class hierarchies.

  • Use attr_accessor, attr_reader, and attr_writer to define trivial accessors and mutators.

    # bad
    class Person
      def initialize(first_name, last_name)
        @first_name = first_name
        @last_name = last_name
      end
    
      def first_name
        @first_name
      end
    
      def last_name
        @last_name
      end
    end
    
    # good
    class Person
      attr_reader :first_name, :last_name
    
      def initialize(first_name, last_name)
        @first_name = first_name
        @last_name = last_name
      end
    end
  • Prefer attr_reader and attr_accessor over attr.

  • Avoid class (@@) variables.

  • Indent the public, protected, and private methods as much as the method definitions they apply to. Leave one blank line above the visibility modifier and one blank line below it.

    class SomeClass
      def public_method
        # ...
      end
    
      private
    
      def private_method
        # ...
      end
    
      def another_private_method
        # ...
      end
    end
  • Prefer alias_method over alias.

Exceptions

  • Signal exceptions using the raise method.

  • Omit RuntimeError in the two argument version of raise.

    # bad
    raise RuntimeError, "message"
    
    # good - signals a RuntimeError by default
    raise "message"
  • Prefer supplying an exception class and a message as two separate arguments to raise instead of an exception instance.

    # bad
    raise SomeException.new("message")
    # Note that there is no way to do `raise SomeException.new("message"), backtrace`.
    
    # good
    raise SomeException, "message"
    # Consistent with `raise SomeException, "message", backtrace`.
  • Avoid returning from an ensure block. If you explicitly return from a method inside an ensure block, the return will take precedence over any exception being raised, and the method will return as if no exception had been raised at all. In effect, the exception will be silently thrown away.

    # bad
    def foo
      raise
    ensure
      return "very bad idea"
    end
  • Use implicit begin blocks where possible.

    # bad
    def foo
      begin
        # main logic goes here
      rescue
        # failure handling goes here
      end
    end
    
    # good
    def foo
      # main logic goes here
    rescue
      # failure handling goes here
    end
  • Avoid empty rescue statements.

    # bad
    begin
      # an exception occurs here
    rescue SomeError
      # the rescue clause does absolutely nothing
    end
    
    # bad - `rescue nil` swallows all errors, including syntax errors, and
    # makes them hard to track down.
    do_something rescue nil
  • Avoid rescue in its modifier form.

    # bad - this catches exceptions of StandardError class and its descendant
    # classes.
    read_file rescue handle_error($!)
    
    # good - this catches only the exceptions of Errno::ENOENT class and its
    # descendant classes.
    def foo
      read_file
    rescue Errno::ENOENT => error
      handle_error(error)
    end
  • Avoid rescuing the Exception class.

    # bad
    begin
      # calls to exit and kill signals will be caught (except kill -9)
      exit
    rescue Exception
      puts "you didn't really want to exit, right?"
      # exception handling
    end
    
    # good
    begin
      # a blind rescue rescues from StandardError, not Exception.
    rescue => error
      # exception handling
    end
  • Prefer exceptions from the standard library over introducing new exception classes.

  • Use meaningful names for exception variables.

    # bad
    begin
      # an exception occurs here
    rescue => e
      # exception handling
    end
    
    # good
    begin
      # an exception occurs here
    rescue => error
      # exception handling
    end

Collections

  • Use literal array and hash creation notation unless you need to pass parameters to their constructors.

    # bad
    arr = Array.new
    hash = Hash.new
    
    # good
    arr = []
    hash = {}
  • Prefer the literal array syntax over %w or %i.

    # bad
    STATES = %w(draft open closed)
    
    # good
    STATES = ["draft", "open", "closed"]
  • Append a trailing comma in multi-line collection literals.

    # bad
    {
      foo: :bar,
      baz: :toto
    }
    
    # good
    {
      foo: :bar,
      baz: :toto,
    }
  • When accessing the first or last element from an array, prefer first or last over [0] or [-1].

  • Avoid mutable objects as hash keys.

  • Use shorthand hash literal syntax when all keys are symbols.

    # bad
    { :a => 1, :b => 2 }
    
    # good
    { a: 1, b: 2 }
  • Prefer hash rockets syntax over shorthand syntax when not all keys are symbols.

    # bad
    { a: 1, "b" => 2 }
    
    # good
    { :a => 1, "b" => 2 }
  • Prefer Hash#key? over Hash#has_key?.

  • Prefer Hash#value? over Hash#has_value?.

  • Use Hash#fetch when dealing with hash keys that should be present.

    heroes = { batman: "Bruce Wayne", superman: "Clark Kent" }
    # bad - if we make a mistake we might not spot it right away
    heroes[:batman] # => "Bruce Wayne"
    heroes[:supermann] # => nil
    
    # good - fetch raises a KeyError making the problem obvious
    heroes.fetch(:supermann)
  • Introduce default values for hash keys via Hash#fetch as opposed to using custom logic.

    batman = { name: "Bruce Wayne", is_evil: false }
    
    # bad - if we just use || operator with falsy value we won't get the expected result
    batman[:is_evil] || true # => true
    
    # good - fetch work correctly with falsy values
    batman.fetch(:is_evil, true) # => false
  • Place ] and } on the line after the last element when opening brace is on a separate line from the first element.

    # bad
    [
      1,
      2]
    
    {
      a: 1,
      b: 2}
    
    # good
    [
      1,
      2,
    ]
    
    {
      a: 1,
      b: 2,
    }

Strings

  • Prefer string interpolation and string formatting instead of string concatenation:

    # bad
    email_with_name = user.name + " <" + user.email + ">"
    
    # good
    email_with_name = "#{user.name} <#{user.email}>"
    
    # good
    email_with_name = format("%s <%s>", user.name, user.email)
  • Avoid padded-spacing inside braces in interpolated expressions.

    # bad
    "From: #{ user.first_name }, #{ user.last_name }"
    
    # good
    "From: #{user.first_name}, #{user.last_name}"
  • Use double-quoted strings.

    # bad
    'Just some text'
    'No special chars or interpolation'
    
    # good
    "Just some text"
    "No special chars or interpolation"
    "Every string in #{project} uses double_quotes"
  • Avoid the character literal syntax ?x.

  • Use {} around instance and global variables being interpolated into a string.

    class Person
      attr_reader :first_name, :last_name
    
      def initialize(first_name, last_name)
        @first_name = first_name
        @last_name = last_name
      end
    
      # bad - valid, but awkward
      def to_s
        "#@first_name #@last_name"
      end
    
      # good
      def to_s
        "#{@first_name} #{@last_name}"
      end
    end
    
    $global = 0
    # bad
    puts "$global = #$global"
    
    # fine, but don't use globals
    puts "$global = #{$global}"
  • Avoid Object#to_s on interpolated objects.

    # bad
    message = "This is the #{result.to_s}."
    
    # good - `result.to_s` is called implicitly.
    message = "This is the #{result}."
  • Avoid String#gsub in scenarios in which you can use a faster more specialized alternative.

    url = "http://example.com"
    str = "lisp-case-rules"
    
    # bad
    url.gsub("http://", "https://")
    str.gsub("-", "_")
    str.gsub(/[aeiou]/, "")
    
    # good
    url.sub("http://", "https://")
    str.tr("-", "_")
    str.delete("aeiou")
  • When using heredocs for multi-line strings keep in mind the fact that they preserve leading whitespace. It's a good practice to employ some margin based on which to trim the excessive whitespace.

    code = <<-END.gsub(/^\s+\|/, "")
      |def test
      |  some_method
      |  other_method
      |end
    END
    # => "def test\n  some_method\n  other_method\nend\n"
    
    # In Rails you can use `#strip_heredoc` to achieve the same result
    code = <<-END.strip_heredoc
      def test
        some_method
        other_method
      end
    END
    # => "def test\n  some_method\n  other_method\nend\n"
  • In Ruby 2.3, prefer "squiggly heredoc" syntax, which has the same semantics as strip_heredoc from Rails:

    code = <<~END
      def test
        some_method
        other_method
      end
    END
    # => "def test\n  some_method\n  other_method\nend\n"
  • Indent heredoc contents and closing according to its opening.

    # bad
    class Foo
      def bar
        <<~SQL
          'Hi'
      SQL
      end
    end
    
    # good
    class Foo
      def bar
        <<~SQL
          'Hi'
        SQL
      end
    end
    
    # bad
    
    # heredoc contents is before closing heredoc.
    foo arg,
        <<~EOS
      Hi
        EOS
    
    # good
    foo arg,
        <<~EOS
      Hi
    EOS
    
    # good
    foo arg,
      <<~EOS
        Hi
      EOS

Regular Expressions

  • Prefer plain text search over regular expressions in strings.

    string["text"]
  • Use non-capturing groups when you don't use the captured result.

    # bad
    /(first|second)/
    
    # good
    /(?:first|second)/
  • Prefer Regexp#match over Perl-legacy variables to capture group matches.

    # bad
    /(regexp)/ =~ string
    process $1
    
    # good
    /(regexp)/.match(string)[1]
  • Prefer named groups over numbered groups.

    # bad
    /(regexp)/ =~ string
    ...
    process Regexp.last_match(1)
    
    # good
    /(?<meaningful_var>regexp)/ =~ string
    ...
    process meaningful_var
  • Prefer \A and \z over ^ and $ when matching strings from start to end.

    string = "some injection\nusername"
    string[/^username$/] # `^` and `$` matches start and end of lines.
    string[/\Ausername\z/] # `\A` and `\z` matches start and end of strings.

Percent Literals

  • Use %() for single-line strings which require both interpolation and embedded double-quotes. For multi-line strings, prefer heredocs.

  • Avoid %q unless you have a string with both ' and " in it. Regular string literals are more readable and should be preferred unless a lot of characters would have to be escaped in them.

  • Use %r only for regular expressions matching at least one / character.

    # bad
    %r{\s+}
    
    # good
    %r{^/(.*)$}
    %r{^/blog/2011/(.*)$}
  • Avoid the use of %s. Use :"some string" to create a symbol with spaces in it.

  • Prefer () as delimiters for all % literals, except, as often occurs in regular expressions, when parentheses appear inside the literal. Use the first of (), {}, [], <> which does not appear inside the literal.

Testing

  • Treat test code like any other code you write. This means: keep readability, maintainability, complexity, etc. in mind.

  • Prefer Minitest as the test framework.

  • Limit each test case to cover a single aspect of your code.

  • Organize the setup, action, and assertion sections of the test case into paragraphs separated by empty lines.

    test "sending a password reset email clears the password hash and set a reset token" do
      user = User.create!(email: "[email protected]")
      user.mark_as_verified
    
      user.send_password_reset_email
    
      assert_nil user.password_hash
      refute_nil user.reset_token
    end
  • Split complex test cases into multiple simpler tests that test functionality in isolation.

  • Prefer using test "foo"-style syntax to define test cases over def test_foo.

  • Prefer using assertion methods that will yield a more descriptive error message.

    # bad
    assert user.valid?
    assert user.name == "tobi"
    
    
    # good
    assert_predicate user, :valid?
    assert_equal "tobi", user.name
  • Avoid using assert_nothing_raised. Use a positive assertion instead.

  • Prefer using assertions over expectations. Expectations lead to more brittle tests, especially in combination with singleton objects.

    # bad
    StatsD.expects(:increment).with("metric")
    do_something
    
    # good
    assert_statsd_increment("metric") do
      do_something
    end

ruby-style-guide's People

Contributors

adrianna-chang-shopify avatar airhorns avatar andyw8 avatar burke avatar cbothner avatar dependabot-preview[bot] avatar dependabot[bot] avatar dylanahsmith avatar eapache avatar edouard-chin avatar eljojo avatar etiennebarrie avatar github-actions[bot] avatar gmalette avatar gmcgibbon avatar kmcphillips avatar korri avatar larouxn avatar lavoiesl avatar lucasuyezu avatar meagar avatar miry avatar nunosilva800 avatar rafaelfranca avatar sambostock avatar schwad avatar sergey-alekseev avatar simon-shopify avatar volmer avatar wvanbergen avatar

Stargazers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

Watchers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

ruby-style-guide's Issues

Layout/LineLength obsolete parameter `IgnoredPatterns`

Just noticed there's a warning showing up while running rubocop due to a deprecated(?) IgnoredPatterns param.

❯ rubocop
Warning: obsolete parameter `IgnoredPatterns` (for `Layout/LineLength`) found in /Users/larouxn/.gem/ruby/3.1.2/gems/rubocop-shopify-2.5.0/rubocop.yml
`IgnoredPatterns` has been renamed to `AllowedPatterns`.
Warning: obsolete parameter `IgnoredPatterns` (for `Layout/LineLength`) found in .rubocop.yml
`IgnoredPatterns` has been renamed to `AllowedPatterns`.
Inspecting 2166 files

Layout/LineLength:
IgnoreCopDirectives: false
IgnoredPatterns:
- "\\A\\s*(remote_)?test(_\\w+)?\\s.*(do|->)(\\s|\\Z)"

Consider re-evaluating Style/ModuleFunction: extend_self

Has the decision to enforce extend_self over module_function ever been revisited internally?

The whole point of module_function is to allow namespaced stateless methods to be available as public class methods, while simultaneously included as private helper methods (when those methods are oft-used in a particular class) without polluting the class' public api. extend self isn't compatible with module_function because the visibility of the methods is forced to be the same at both the class and instance level.

When extend self; private is used to avoid polluting the class' instance api, then you can't invoke them statically. But if you make them public so they can be namespaced utility methods, then they pollute the class' public api!

This implies these two approaches aren't solving the same problem. module_function is purpose built to have different visibility. extend self is an idiom that is used for a variety of other purposes that only appears to overlap with module_function. But it has a bigger ramification than was likely recognized when it become oft-used/cargo-culted.

I dug into the history of the cop's setting. It seems the initial decision was at least partially (if not primarily?) about class << self vs def self.. Nothing in the issue thread speaks to the nuance that module_function provides; wherein methods are desired to have different visibilities because they serve different purposes (public as class; private as instance). Since the initial discussion didn't seem to take these different purposes and behaviors into account, I'm hoping to at least broach that topic here.

Time (and Date) doesn't always have equivalents of DateTime methods

Encountered a few issues trying to fix the style for spy. Those seem relatively minor but each change needs a lot more reflexion than I expected.

Time.parse doesn't end up with the same time zone

The 00:00 timezone information is not parsed the same way. Time seems to ignore the time zone information from the argument in this case (local time zone is currently UTC-4):

[2] pry(main)> DateTime.parse('2017-08-08T15:00:00 00:00').utc.iso8601
=> "2017-08-08T15:00:00+00:00"
[3] pry(main)> Time.parse('2017-08-08T15:00:00 00:00').utc.iso8601
=> "2017-08-08T19:00:00Z"

In this case Time.zone.parse is a more suitable replacement for DateTime.rfc3339.

Time.iso8601 prints UTC time zone differently

[1] pry(main)> DateTime.parse('2017-08-08T15:00:00Z').iso8601
=> "2017-08-08T15:00:00+00:00"
[2] pry(main)> Time.parse('2017-08-08T15:00:00Z').iso8601
=> "2017-08-08T15:00:00Z"

This is mostly harmless, but sometimes makes tests fail.

Date.rfc3339 timezone information is lost

[4] pry(main)> DateTime.rfc3339('2002-10-02T10:00:00-05:00').to_time.utc
=> 2002-10-02 15:00:00 UTC
[5] pry(main)> Date.rfc3339('2002-10-02T10:00:00-05:00').to_time.utc
=> 2002-10-02 04:00:00 UTC

Again, Time.zone.parse is a more suitable replacement for DateTime.rfc3339.

Suggestion: enforce trailing commas in literals

Right now we have this rule disabled: http://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/TrailingCommaInLiteral

This cop checks for trailing comma in array and hash literals.

This is something that frequently gets called out in PRs as trailing commas are generally accepted as a good practice for multiline literals. Right now we have nothing enforcing it one way or another.

Is there a compelling reason why we shouldn't use this rule to enforce the usage?

cc @nickhoffman

Autoformat issues with v2.11.1

I have updated rubocop-shopify to 2.11.1 and fixing the layout with bundle exec rubocop -A produced some weird changes.

The code before:

    def collection
      Service.not_deleted.includes(
        { vault_teams:
            [:github_team_vault_teams, :teams] },
        { app:
            [{ runtimes:
                 [:runtime_instances, :app] },
             :runtimes,
             :repo,] },
        :repo,
        :app,
        :monorepo_mobile_app,
        :bugsnag_project,
        :slack_channels,
        :vault_teams,
        :vault_users,
      ).joins(:app)
    end

The code after:

    def collection
      Service.not_deleted.includes(
        {
          vault_teams:
                      [:github_team_vault_teams, :teams],
        },
        {
          app:
                      [
                        {
                          runtimes:
                                           [:runtime_instances, :app],
                        },
                        :runtimes,
                        :repo,
                      ],
        },
        :repo,
        :app,
        :monorepo_mobile_app,
        :bugsnag_project,
        :slack_channels,
        :vault_teams,
        :vault_users,
      ).joins(:app)
    end

There are more extreme examples in this file.

It seems the problem doesn't happen if the opening bracket (or whole entry) is on the same line:

Before

    def collection
      Service.not_deleted.includes(
        { vault_teams:[:github_team_vault_teams, :teams] },
        { app:[
          { runtimes: [:runtime_instances, :app] },
             :runtimes,
             :repo,] },
        :repo,
        :app,
        :monorepo_mobile_app,
        :bugsnag_project,
        :slack_channels,
        :vault_teams,
        :vault_users,
      ).joins(:app)
    end

After

    def collection
      Service.not_deleted.includes(
        { vault_teams: [:github_team_vault_teams, :teams] },
        {
          app: [
            { runtimes: [:runtime_instances, :app] },
            :runtimes,
            :repo,
          ],
        },
        :repo,
        :app,
        :monorepo_mobile_app,
        :bugsnag_project,
        :slack_channels,
        :vault_teams,
        :vault_users,
      ).joins(:app)
    end

Cut a release to eliminate rubocop 1.4 warning?

Hi friends!

Just updated rubocop, and I now get a warning because Style/ArrayIntersect is not configured. I could configure it on my side but would rather just have a release of ruby-style-guide that does it for me (as done by @sambostock in #487).

If you could cut a new release, that would be most appreciated!

Include reasoning in some rules

Extracted from #74. It’s not clear however which rules require the « why » to be explicit in the style guide. I fear that explaining the whole context behind a rule would make the document way too long, and developers can access the PR discussions to learn more about it via git blame.

cc @sambostock

Proposition: Disable `Style/CaseEquality`

We currently have Style/CaseEquality cop enabled, which disallows the usage of ===.

In general, I agree that === should be discouraged (on Enumerable, Regexp, etc), but I don't think we can disallow it using Rubocop, and in at least one scenario it should actually be the encouraged way of doing things: hierarchy membership checks (aka: typechecking, but not quite).

The idiomatic way of doing hierarchy membership checking is typically using the is_a? or kind_of?, but this should actually be discouraged, for two reasons.

  1. BasicObject doesn't respond to is_a? and so whenever we use is_a?.
    When we use is_a? we introduce coupling (which is generally unnecessary), between the code that checks and the code that produces the objects. This practice is also prevented by Sorbet when using interfaces, because the implementation could very well be a BasicObject. In the Checkouts component, we've had to disable the check entirely since we use Sorbet with strict typing enabled.

  2. When checking membership, we should always ask the collection rather than the item. E.g., we do array.include?(item) and never item.contained_in?(array).


Continuing on a tangent...

I think we should disable all Rubocop rules that can have different semantics. Rubocop is simply not equipped to know what the right solution should be. It's fine to Rubocop to indent code, add commas, or parentheses, or make sure that a class name fits the file name. However, it shouldn't be used to know which method to use. Another great backwards example is the DynamicFinder cop. Sometimes, your API will have def find_by_id, and it's not an ActiveRecord, so it shouldn't be replaced by find_by(id:). If we used a linter that was aware of the types, we could pull this off, but Rubocop isn't aware of it, and IMO, shouldn't be used here.

Should we expose our style guide as a gem?

TL;DR

It might make sense for us to expose the styleguide as a gem, allowing configuration updates to become part of the predicatable Dependabot auto-update process.


Details

As brought up in Slack

Currently, the way we share our Rubocop config across repos is

# .rubocop.yml
inherit_from:
  - http://shopify.github.io/ruby-style-guide/rubocop.yml

The thing is, this doesn't follow any kind of versioning. It just follows the rules from inheriting from a remote URL

The remote config file is cached locally and is only updated if:

  • The file does not exist.
  • The file has not been updated in the last 24 hours.
  • The remote copy has a newer modification time than the local copy.

Sometimes the file is gitignored, sometimes it isn't.

  • If it is ignored, then sometimes new rules start being enforced in CI.
  • If it isn't, then sometimes a random PR will have updates to the style config.

Given that we have Dependabot to keep dependencies up to date, and it is possible to include Rubocop config in a gem, would it make sense to publish our config as a gem, and use inherit_gem to apply it?

Other benefits that come to mind are:

  • We could explicitly make Rubocop a dependency.
    No more incompatible cops due to version mismatches!
  • We could use existing dependency tooling to check for up-to-date style configs (e.g. Services Internal classification checks)
  • We could add dependencies on the extracted cop gems (e.g. rubocop-performance), without requiring the client to know to install those cops

If we wanted to get really bold, maybe a gem would even be a good place to put the common foundation for a command similar to dev style --include-branch-commits, instead of every repo implementing it on their own.


cc. @rafaelfranca

Style guide style improvements

Not a huge deal, but I thought I'd throw this out there.

I really like the way AirBnB does their Javascript Style Guide, for the following reasons:

  • Table of Contents 📄
  • Linking to specific sections 👈
  • Examples for almost everything, often with 😇 good vs 👿 bad
  • "Why"s on many style rules 🤔

In my opinion, these make the style guide much more approachable to developers new to a language, as well as making the rules easier to share with others/refer to. I like having a "why" too, because rules make so much more sense when they aren't arbitrary.

Recommendation on Style/ArrayIntersect?

In some of our repos we are starting to get prompted for adding the Style/ArrayIntersect cop. Seems like this new cop was added fairly recently (v1.40):

The following cops were added to RuboCop, but are not configured. Please set Enabled to either `true` or `false` in your `.rubocop.yml` file.

Please also note that you can opt-in to new cops by default by adding this to your config:
  AllCops:
    NewCops: enable

Style/ArrayIntersect: # new in 1.40
  Enabled: true

What is the recommendation for adding new cops not covered by our centralized rubocop.yml styleguide? This came up after a recent dependabot updates for rubocop-shopify

Gemspec/DateAssignment cop has been removed.

Ruby Version: 2.6.9

GemEnv

RubyGems Environment:
  - RUBYGEMS VERSION: 3.0.3.1
  - RUBY VERSION: 2.6.9 (2021-11-24 patchlevel 207) [x86_64-linux]
  - INSTALLATION DIRECTORY: /home/lance/.gem/ruby/2.6.9
  - USER INSTALLATION DIRECTORY: /home/lance/.gem/ruby/2.6.0
  - RUBY EXECUTABLE: /home/lance/.rubies/ruby-2.6.9/bin/ruby
  - GIT EXECUTABLE: /usr/bin/git
  - EXECUTABLE DIRECTORY: /home/lance/.gem/ruby/2.6.9/bin
  - SPEC CACHE DIRECTORY: /home/lance/.gem/specs
  - SYSTEM CONFIGURATION DIRECTORY: /home/lance/.rubies/ruby-2.6.9/etc
  - RUBYGEMS PLATFORMS:
    - ruby
    - x86_64-linux
  - GEM PATHS:
     - /home/lance/.gem/ruby/2.6.9
     - /home/lance/.rubies/ruby-2.6.9/lib/ruby/gems/2.6.0
  - GEM CONFIGURATION:
     - :update_sources => true
     - :verbose => true
     - :backtrace => false
     - :bulk_threshold => 1000
     - :sources => ["https://rubygems.org/"]
     - :concurrent_downloads => 8
     - "gem" => "--document=yri"
  - REMOTE SOURCES:
     - https://rubygems.org/
  - SHELL PATH:
     - /home/lance/.gem/ruby/2.6.9/bin
     - /home/lance/.rubies/ruby-2.6.9/lib/ruby/gems/2.6.0/bin
     - /home/lance/.rubies/ruby-2.6.9/bin
     - /home/lance/.local/bin
     - /home/lance/.nvm/versions/node/v14.19.1/bin
     - /home/lance/.local/bin
     - /usr/local/sbin
     - /usr/local/bin
     - /usr/sbin
     - /usr/bin
     - /sbin
     - /bin
     - /usr/games
     - /usr/local/games
     - /snap/bin
     - /home/lance/dev/bin/

Added just this gem (no other rubocop gems) to my project and get this error.

bundle exec rubocop
Error: The `Gemspec/DateAssignment` cop has been removed. Please use `Gemspec/DeprecatedAttributeAssignment` instead.
(obsolete configuration found in /home/lance/.gem/ruby/2.6.9/gems/rubocop-shopify-2.5.0/rubocop.yml, please update it)


gem list rubocop      

*** LOCAL GEMS ***

rubocop (1.31.0)
rubocop-ast (1.24.0)
rubocop-rspec (2.11.1)
rubocop-shopify (2.5.0)

Move to make Policial fails a hard fail

Is there a plan for making Policial no longer advisory and actually something that needs to pass before merging? If not I think there should be.

  • It's important to be able to know if your PR is safe to merge at a glance. We want to be able to ship fast, and having to click in to see what the issues are is slow. For the same reasons that false CI failures are so annoying, false lint failures should be just as important.
  • It's important to have a consistently applied set of style rules. Anecdotally, it seems like everyone has their own set of rules they consider actual rules, and a bunch more that they consider guidelines and will still merge when red. These two sets of rules are a little different for each person. So, we're tending towards writing similar code, but never quite making it.
  • It's important for onboarding to not have broken windows and tribal knowledge. Which rules are rules and which are guidelines is something people just seem to make up, and this is intimidating / unknowable for people new to the company. 30% of developers joined in the past 5 months.

@volmer @kirs ... who else should I tag?

Add more good vs bad examples

Extracted from #74. It would be great to know exactly which rules would be made easier to understand with more examples. Some are simple and straightforward enough and don’t need examples.

cc @sambostock

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.