From 15ef7afc3dbdefab8f134afc197215952349d796 Mon Sep 17 00:00:00 2001 From: Aaron Kromer Date: Wed, 13 Jun 2018 19:48:05 -0400 Subject: [PATCH] Bump Rubocop to 0.57.x 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 https://github.com/rubocop-hq/rubocop/issues/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. --- common_rubocop_rails.yml | 7 ------- radius-spec.gemspec | 2 +- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/common_rubocop_rails.yml b/common_rubocop_rails.yml index 17fe56f..2ef016f 100644 --- a/common_rubocop_rails.yml +++ b/common_rubocop_rails.yml @@ -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? diff --git a/radius-spec.gemspec b/radius-spec.gemspec index 61649c0..a5ac3f3 100644 --- a/radius-spec.gemspec +++ b/radius-spec.gemspec @@ -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"