Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unset_nil option to ConfigLoader.merge_with_default #7048

Merged

Conversation

koic
Copy link
Member

@koic koic commented May 15, 2019

Summary

Follow up #6942.

unset_nil was introduced to RuboCop core at #6942.

This causes the following warning to be displayed for TargetRailsVersion and Database where null (or ~) is specified in the RuboCop Rails config/default.yml configuration.

% cd path/to/rubocop-rails
% bundle exec rake

(snip)

Warning: AllCops does not support TargetRailsVersion parameter.

Supported parameters are:

  - RubyInterpreters
  - Include
  - Exclude
  - DefaultFormatter
  - DisplayCopNames
  - DisplayStyleGuide
  - StyleGuideBaseURL
  - ExtraDetails
  - StyleGuideCopsOnly
  - EnabledByDefault
  - DisabledByDefault
  - UseCache
  - MaxFilesInCache
  - CacheRootDirectory
  - AllowSymlinksInCacheRootDirectory
  - TargetRubyVersion

Warning: Rails/BulkChangeTable does not support Database parameter.

Supported parameters are:

  - Enabled
  - SupportedDatabases
  - Include

This PR aims to prevent this issue by the following work around to RuboCop Rails.
I'm going to open a PR to RuboCop Rails if this PR is accepted.

diff --git a/lib/rubocop/rails/inject.rb b/lib/rubocop/rails/inject.rb
index abb715ea7..8d1ffa371 100644
--- a/lib/rubocop/rails/inject.rb
+++ b/lib/rubocop/rails/inject.rb
@@ -10,7 +10,7 @@ module RuboCop
         hash = ConfigLoader.send(:load_yaml_configuration, path)
         config = Config.new(hash, path)
         puts "configuration from #{path}" if ConfigLoader.debug?
-        config = ConfigLoader.merge_with_default(config, path)
+        config = ConfigLoader.merge_with_default(config, path, unset_nil: false)
         ConfigLoader.instance_variable_set(:@default_configuration, config)
       end
     end

Other Information

The following example app code reproduces this issue using RuboCop Rails master code.

# Gemfile
source "https://rubygems.org"

git_source(:github) {|repo_name| "https://github.com/#{repo_name}" }

gem 'rubocop', github: 'rubocop-hq/rubocop'
gem 'rubocop-rails', github: 'rubocop-hq/rubocop-rails'
# app/models/user.rb
class User < ActiveRecord::Base
end
# .rubocop.yml
AllCops:
  TargetRailsVersion: 5.2
% bunlde install
% bundle exec rubocop --only Rails/ApplicationRecord --require rubocop-rails
Warning: AllCops does not support TargetRailsVersion parameter.

Supported parameters are:

  - RubyInterpreters
  - Include
  - Exclude
  - DefaultFormatter
  - DisplayCopNames
  - DisplayStyleGuide
  - StyleGuideBaseURL
  - ExtraDetails
  - StyleGuideCopsOnly
  - EnabledByDefault
  - DisabledByDefault
  - UseCache
  - MaxFilesInCache
  - CacheRootDirectory
  - AllowSymlinksInCacheRootDirectory
  - TargetRubyVersion

Inspecting 2 files
.C

Offenses:

app/models/user.rb:1:14: C: Rails/ApplicationRecord: Models should
subclass ApplicationRecord.
class User < ActiveRecord::Base
             ^^^^^^^^^^^^^^^^^^

2 files inspected, 1 offense detected

This means that an unexpected warning is displayed during a migration period until RuboCop Rails cops is removed from RuboCop core.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

### Summary

Follow up rubocop#6942.

`unset_nil` was introduced to RuboCop core at rubocop#6942.

This causes the following warning to be displayed for
`TargetRailsVersion` and `Database` where `null` (or `~`) is
specified in the RuboCop Rails `config/default.yml` configuration.

- https://github.com/rubocop-hq/rubocop-rails/blob/5180fa2c25cef467eececd9c80739f7cad6adbbf/config/default.yml#L11
- https://github.com/rubocop-hq/rubocop-rails/blob/5180fa2c25cef467eececd9c80739f7cad6adbbf/config/default.yml#L89

```console
% cd path/to/rubocop-rails
% bundle exec rake

(snip)

Warning: AllCops does not support TargetRailsVersion parameter.

Supported parameters are:

  - RubyInterpreters
  - Include
  - Exclude
  - DefaultFormatter
  - DisplayCopNames
  - DisplayStyleGuide
  - StyleGuideBaseURL
  - ExtraDetails
  - StyleGuideCopsOnly
  - EnabledByDefault
  - DisabledByDefault
  - UseCache
  - MaxFilesInCache
  - CacheRootDirectory
  - AllowSymlinksInCacheRootDirectory
  - TargetRubyVersion

Warning: Rails/BulkChangeTable does not support Database parameter.

Supported parameters are:

  - Enabled
  - SupportedDatabases
  - Include
```

This PR aims to prevent this issue by the following work around to RuboCop Rails.
I will open a PR to RuboCop Rails if this PR is accepted.

```diff
diff --git a/lib/rubocop/rails/inject.rb b/lib/rubocop/rails/inject.rb
index abb715ea7..8d1ffa371 100644
--- a/lib/rubocop/rails/inject.rb
+++ b/lib/rubocop/rails/inject.rb
@@ -10,7 +10,7 @@ module RuboCop
         hash = ConfigLoader.send(:load_yaml_configuration, path)
         config = Config.new(hash, path)
         puts "configuration from #{path}" if ConfigLoader.debug?
-        config = ConfigLoader.merge_with_default(config, path)
+        config = ConfigLoader.merge_with_default(config, path, unset_nil: false)
         ConfigLoader.instance_variable_set(:@default_configuration, config)
       end
     end
```

### Other Information

The following example app code reproduces this issue using
RuboCop Rails master code.

```ruby
# Gemfile
source "https://rubygems.org"

git_source(:github) {|repo_name| "https://github.com/#{repo_name}" }

gem 'rubocop', github: 'rubocop-hq/rubocop'
gem 'rubocop-rails', github: 'rubocop-hq/rubocop-rails'
```

```ruby
# app/models/user.rb
class User < ActiveRecord::Base
end
```

```yaml
# .rubocop.yml
AllCops:
  TargetRailsVersion: 5.2
```

```
% bunlde install
% bundle exec rubocop --only Rails/ApplicationRecord --require rubocop-rails
Warning: AllCops does not support TargetRailsVersion parameter.

Supported parameters are:

  - RubyInterpreters
  - Include
  - Exclude
  - DefaultFormatter
  - DisplayCopNames
  - DisplayStyleGuide
  - StyleGuideBaseURL
  - ExtraDetails
  - StyleGuideCopsOnly
  - EnabledByDefault
  - DisabledByDefault
  - UseCache
  - MaxFilesInCache
  - CacheRootDirectory
  - AllowSymlinksInCacheRootDirectory
  - TargetRubyVersion

Inspecting 2 files
.C

Offenses:

app/models/user.rb:1:14: C: Rails/ApplicationRecord: Models should
subclass ApplicationRecord.
class User < ActiveRecord::Base
             ^^^^^^^^^^^^^^^^^^

2 files inspected, 1 offense detected
```

This means that an unexpected warning is displayed during a migration
period until RuboCop Rails cops is removed from RuboCop core.
@bbatsov bbatsov merged commit 72402c0 into rubocop:master May 15, 2019
@bbatsov
Copy link
Collaborator

bbatsov commented May 15, 2019

Looks good to me! Thanks!

@koic koic deleted the add_unset_nil_to_config_loader_merge_with_default branch May 15, 2019 06:20
koic added a commit to koic/rubocop-rails that referenced this pull request May 15, 2019
Follow up rubocop/rubocop#7048.

This PR fixes the following warning.

```console
% cd path/to/rubocop-performance
% bundle exec rake generate_cops_documentation
Files:          54
Modules:         4 (    2 undocumented)
Classes:        55 (    0 undocumented)
Constants:     122 (  122 undocumented)
Attributes:      1 (    0 undocumented)
Methods:       117 (  100 undocumented)
 25.08% documented
Warning: AllCops does not support TargetRailsVersion parameter.

Supported parameters are:

  - RubyInterpreters
  - Include
  - Exclude
  - DefaultFormatter
  - DisplayCopNames
  - DisplayStyleGuide
  - StyleGuideBaseURL
  - ExtraDetails
  - StyleGuideCopsOnly
  - EnabledByDefault
  - DisabledByDefault
  - UseCache
  - MaxFilesInCache
  - CacheRootDirectory
  - AllowSymlinksInCacheRootDirectory
  - TargetRubyVersion

Warning: Rails/BulkChangeTable does not support Database parameter.

Supported parameters are:

  - Enabled
  - SupportedDatabases
  - Include

* generated
  /Users/koic/src/github.com/rubocop-hq/rubocop-rails/manual/cops_rails.md
```

Note: Use of `unset_nil: false` option requires a release higher than RuboCop 0.69.0.
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.

None yet

2 participants