Skip to content

Commit

Permalink
Bump Rubocop to 0.57.x
Browse files Browse the repository at this point in the history
This Rubocop version adds several new cops:

  - `Rails/BulkChangeTable`

    I wasn't aware of the bulk change table being a more ideal choice
    due to it's bundling of the operation so nice to see that cop added.
  - `Style/AccessModifierDeclarations`

    This is a controversial style cop. The vast majority of Ruby code
    I've seen, and written, using the grouping style. While this cop
    defaults to that style it was written with the intention of forcing
    the alternative inline style. Additionally, the cop has a [:bug:
    which flags instances which are impossible to write using the inline
    style](see rubocop/rubocop#5953).

    The author of the cop has two arguments against the group style:

      - in big methods someone has to scroll to see the modifier and
        even then it can be lost (as the modifier is indented the same
        level as the methods in their examples)

      - class methods listed under non-public modifiers are still public

    Let me attempt to address the first point. We have set our style to
    outdent access modifiers to better highlight the groupings. However,
    I still admit that large classes can cause such scrolling issues;
    esp. classes which have lots of documentation.

    Conversely, many IDEs have built-in features or plugins available
    which can fold code, shortening the file, and show a sidebar with
    the class method overview including modifier annotations. I
    personally use vim so I don't have that benefit but it does exist.
    Additionally, I would argue that the grouping style better forces
    grouping methods by access which the inline style would not. IMO the
    grouping makes it easier see at a glance how much of an API is
    public; which can affect how quickly I can find something usable
    within the class.

    I also worry, that Ruby programmers, which are used to the grouping
    style (which is sort of the Ruby default) will accidentally make
    things public without realizing it. Since public is the default
    access and this cop does not force an explicit use of `public` in
    the inline style it is a potential risk. Given I haven't used this
    style maybe this isn't really an issue.

    Regarding the second point, there is already a cop which protects
    against that exact issue: `Lint/IneffectiveAccessModifier`. Reading
    the feature discussion there was a bit of talk around this. The
    argument in-favor of the inline style helping to avoid it was
    largely framed as:

      > If `Lint/IneffectiveAccessModifier` is disabled this cop does
      > both. So you just need this cop.

    I am generally in-favor of styles which double as error protection
    and/or contribute additional semantic meaning. However, I'm not
    ready to get behind the inline style yet. Part of me feels the main
    complaint point, regarding scrolling in large files, is more an
    attempt to ease a symptom of violating SRP / composition.

    Also, after reviewing several of our projects there are virtually
    zero violations of the grouping style. Changing to force the inline
    style would be a massive undertaking (as the cop does not support
    autocorrect).

    NOTE: We have one project that mixes styles. As I am the main author
    of the code mixing styles I am leaving this cop enabled with the
    default grouping for now until I can better audit the impact.

  - `Style/UnneededCondition`
  - `Layout/ClosingHeredocIndentation`
  - `Layout/LeadingBlankLines`

This version also includes a fix for `Rails/HttpPositionalArguments`
which supports `headers` and `env` in the kwargs.
  • Loading branch information
cupakromer committed Jun 14, 2018
1 parent 8c0ec2a commit 15ef7af
Show file tree
Hide file tree
Showing 2 changed files with 1 addition and 8 deletions.
7 changes: 0 additions & 7 deletions common_rubocop_rails.yml
Expand Up @@ -59,13 +59,6 @@ Rails/ApplicationRecord:
Rails/CreateTableWithTimestamps:
Enabled: false

# Disabling this as it is broken for request specs which use custom headers.
#
# Configuration parameters: Include.
# Include: spec/**/*, test/**/*
Rails/HttpPositionalArguments:
Enabled: false

# The ActiveSupport monkey patches for `present?` are nearly all defiend as:
#
# !blank?
Expand Down
2 changes: 1 addition & 1 deletion radius-spec.gemspec
Expand Up @@ -31,7 +31,7 @@ Gem::Specification.new do |spec|
spec.required_ruby_version = ">= 2.5"

spec.add_runtime_dependency "rspec", "~> 3.7"
spec.add_runtime_dependency "rubocop", "~> 0.56.0"
spec.add_runtime_dependency "rubocop", "~> 0.57.0"

spec.add_development_dependency "bundler", "~> 1.16"
spec.add_development_dependency "rake", "~> 12.0"
Expand Down

0 comments on commit 15ef7af

Please sign in to comment.