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.

Sign up for GitHub

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

Jump to bottom

[Fix #6926] Allow nil values to unset config defaults #6942

Merged
merged 1 commit into from Apr 17, 2019
Merged

[Fix #6926] Allow nil values to unset config defaults #6942

merged 1 commit into from Apr 17, 2019

Conversation

dduugg
Copy link
Contributor

@dduugg dduugg commented Apr 17, 2019

This PR adds the ability to unset default configuration values by using an explicit nil value (or ~, in YAML). This is consistent with the documented behavior of hash inheritance.

Care must be taken when merging hashes that this only happens when merging into the default config hash, which is done by adding an opt to ConfigLoaderResolver#merge_with_default that is only enabled when calling from ConfigLoaderResolver#merge_with_default. (Otherwise, merging a config hash B with an explicit nil into A would leave an unset value for the config. When this merged hash is merged into the default config, the default value would be unchanged.)

Resolves #6926

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.

@dduugg dduugg changed the title Use nil values to unset defaults [Fix #6926] Allow nil values to unset config defaults Apr 17, 2019
@bbatsov
Copy link
Collaborator

bbatsov commented Apr 17, 2019

The build seems stuck. Can you rebase on master to trigger a new build?

@dduugg dduugg force-pushed the enable-config-unset branch 3 times, most recently from 5dfd3b8 to 0697057 Compare April 17, 2019 12:16
@dduugg
Copy link
Contributor Author

dduugg commented Apr 17, 2019

@bbatsov thanks for your attention, done!

@bbatsov bbatsov merged commit fc0fe57 into rubocop:master Apr 17, 2019
@dduugg dduugg deleted the enable-config-unset branch April 17, 2019 13:23
koic added a commit to koic/rubocop that referenced this pull request May 15, 2019
@koic
Add unset_nil option to ConfigLoader.merge_with_default
07103f2 
### 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.
@koic koic mentioned this pull request May 15, 2019
Merged
8 tasks
bbatsov pushed a commit that referenced this pull request May 15, 2019
@koic @bbatsov
Add unset_nil option to ConfigLoader.merge_with_default
72402c0 
### 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.

- 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot unset default InverseMethods in Style/InverseMethods
2 participants
@dduugg @bbatsov