Skip to content

Conversation

tjschuck
Copy link
Contributor

@tjschuck tjschuck commented May 17, 2017

Right now, CI is failing for all pull requests because of Rubocop failures. This PR is three tiers of "fix" to that issue in three commits:

  1. d9b69c4 — Rubocop itself was failing to even run because of out-of-date parameter names.

    Before this change, you’d get the following error:

    $ rake rubocop
    Running RuboCop...
    Error: obsolete parameter IndentWhenRelativeTo (for Style/CaseIndentation) found in /Users/tjschuck/Code/t/.rubocop.yml
    `IndentWhenRelativeTo` has been renamed to `EnforcedStyle`
    obsolete parameter AlignWith (for Lint/EndAlignment) found in /Users/tjschuck/Code/t/.rubocop.yml
    `AlignWith` has been renamed to `EnforcedStyleAlignWith`
    RuboCop failed!
    

    [Rebased out because this was done on master in ee80a3c and 64fcb6a]

  2. 4bc49a7 — once Rubocop was fixed to run at all, it reported hundreds of offenses. This commit fixes all the actual "violations" per the Rubocop YML via rake rubocop:auto_correct.

  3. b9d6ad1 — Add two new Rubocop directives to account for defaults that shouldn't be considered issues in this codebase.

    1. Set Metrics/BlockLength as disabled for the spec directory. Prior to this change, you get a bunch of failures like:

      spec/set_spec.rb:5:1: C: Block has too many lines. [138/25]
      describe T::Set do ...
      ^^^^^^^^^^^^^^^^^^
      

      But the blocklength validation doesn’t really make sense when assessing files written with the RSpec DSL, since when written properly, the specs are just very long blocks themselves.

    2. Disable Style/IndentHeredoc. For Ruby 2.3+, the recommendation from Rubocop is just to use <<~ or to use "some library (e.g. ActiveSupport's String#strip_heredoc)" for earlier versions of Ruby.

      However, the places that heredocs are being used are to test the output, and those are actually testing the spacing itself explicitly, so it doesn't make sense to squish the indentation out when it's present in this case anyway.

@tjschuck
Copy link
Contributor Author

And boom, look at that: green CI.

One additional note: I did this as three commits because though (1) and (3) are probably okay and can be cherry-picked if you want, but (2) is potentially controversial. If the maintainers disagree with the style changes in (2), I can always update more rules to make Rubocop still pass. Particularly, the major failure (and fix in commit 2) is the rule %w-literals should be delimited by [ and ] — changing this would just take an update to the Style/PercentLiteralDelimiters rule.

Product of running `rake rubocop:auto_correct`
1. Metrics/BlockLength disabled for the spec directory.  Prior to this change, you get a bunch of failures like:

    ```
    spec/set_spec.rb:5:1: C: Block has too many lines. [138/25]
    describe T::Set do ...
    ^^^^^^^^^^^^^^^^^^
    ```

    The blocklength validation doesn’t really make sense when assessing files written with the RSpec DSL, since when written properly, the specs are just very long blocks themselves.

2. Disable Style/IndentHeredoc.  For Ruby 2.3+, the recommendation from Rubocop is just to use `<<~` or to use "some library (e.g. ActiveSupport's String#strip_heredoc)" for earlier versions of Ruby.

    However, the places that heredocs are being used are to test the output, and those are actually testing the spacing itself explicitly, so it doesn't make sense to squish the indentation out when it's present in this case anyway.
@tjschuck
Copy link
Contributor Author

tjschuck commented Dec 7, 2017

Rebased out (1) against master because it was done on master in ee80a3c and 64fcb6a. (2) and (3) still stand, with the same caveat about (2) as above.

CI is now failing for a different non-Rubocop reason, both here and on master.

@tjschuck
Copy link
Contributor Author

tjschuck commented Dec 8, 2017

CI is now failing for a different non-Rubocop reason, both here and on master.

Those other reasons would be fixed by #386, so this PR + that one will lead to green CI on master.

@tjschuck tjschuck closed this Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant