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

Fix inherit_mode for arrays in extension default #9952

Merged
merged 1 commit into from Aug 3, 2021

Conversation

pirj
Copy link
Member

@pirj pirj commented Jul 25, 2021

See rubocop/rubocop-rspec#1126

config/default.yml:

RSpec:
  Language:
    Examples:
      inherit_mode:
        merge:
          - Regular
      Regular:
        - it
        - specify
        - example
        - scenario
        - its

.rubocop.yml:

require:
  - rubocop-rspec

RSpec:
  Language:
    Examples:
      Regular:
        - mycustomexamplealias

Previously, locally-set inherit_mode in extension default configuration was not respected.

I'd love a preliminary review. If you're fine with the approach, I'll add a proper test coverage.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • 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.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@pirj
Copy link
Member Author

pirj commented Jul 25, 2021

@jonas054 Your solution to merging inherit_mode of derived and base is more elegant in merge Exclude by default. Do you want me to adjust this PR to use the same approach?

@pirj
Copy link
Member Author

pirj commented Jul 25, 2021

Even though this is titled as a "fix", it might be considered a breaking change, too.

@bbatsov bbatsov requested a review from jonas054 July 27, 2021 15:56
@bbatsov
Copy link
Collaborator

bbatsov commented Jul 27, 2021

The proposal seems reasonable to me, but I'll defer to @jonas054 on this one. As this affects only extensions and arguably the new behaviour is better I guess we don't have to label this a breaking change.

@dvandersluis
Copy link
Member

Thanks for fixing this @pirj!

I'm good with this not being a breaking change -- even if it does break a config, it is really a bug fix and that config shouldn't have worked in the first place.

Can we add some tests for the change?

Copy link
Collaborator

@jonas054 jonas054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code certainly looks good. Nice and readable. But I too would love to see some tests.

@pirj pirj force-pushed the fix-inherit_mode-for-extensions branch from 560366c to 50cdd28 Compare July 31, 2021 11:46
@pirj pirj requested a review from jonas054 July 31, 2021 11:47
@pirj pirj marked this pull request as ready for review July 31, 2021 11:56
@pirj pirj force-pushed the fix-inherit_mode-for-extensions branch from 50cdd28 to cbee9a6 Compare July 31, 2021 12:06
@bbatsov
Copy link
Collaborator

bbatsov commented Aug 3, 2021

@pirj Something seems off with the Win builds. Can you restart the build?

See rubocop/rubocop-rspec#1126

`config/default.yml`:
```yaml
RSpec:
  Language:
    Examples:
      inherit_mode:
        merge:
          - Regular
      Regular:
        - it
        - specify
        - example
        - scenario
        - its
```

`.rubocop.yml`:
```yaml
require:
  - rubocop-rspec

RSpec:
  Language:
    Examples:
      Regular:
        - mycustomexamplealias
```

Previously, locally-set `inherit_mode` in extension default
configuration was not respected.
@pirj pirj force-pushed the fix-inherit_mode-for-extensions branch from cbee9a6 to 22a1397 Compare August 3, 2021 06:24
@pirj
Copy link
Member Author

pirj commented Aug 3, 2021

@bbatsov All green.
Somehow it fixed itself, I didn't rebase on top of the presumable fix.

@bbatsov bbatsov merged commit 2d9d669 into rubocop:master Aug 3, 2021
@bbatsov
Copy link
Collaborator

bbatsov commented Aug 3, 2021

Thanks!

@pirj pirj deleted the fix-inherit_mode-for-extensions branch August 3, 2021 11:20
@jonas054
Copy link
Collaborator

jonas054 commented Aug 3, 2021

👍

pirj added a commit to rubocop/rubocop-rspec that referenced this pull request Aug 12, 2021
pirj added a commit to rubocop/rubocop-rspec that referenced this pull request Aug 12, 2021
pirj added a commit to rubocop/rubocop-rspec that referenced this pull request Oct 3, 2021
fixes #1126

See palkan/action_policy#103 (comment)

If a third-party gem, e.g. `action_policy`, defines their
`config/default.yml` and supplements to the default RSpec DSL that we
provide in our `config/default.yml`, `rubocop`'s behaviour is to
actually override the configuration, as `inherit_mode` is `override` for
lists by default.

RuboCop has recently [fixed the problem with ignored
`inherit_mode`](rubocop/rubocop#9952) and we've
[bumped to use `rubocop` version that includes the
fix](#1181), but we haven't
adjusted our `config/default.yml` to merge by default.

This is causing both user project RSpec DSL configuration and
third-party gem RSpec DSL configuration to override our default,
rendering our cops blind.

### Example

A new project

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

gem 'action_policy'
gem 'rubocop-rspec'
```

```yml
# .rubocop.yml
require:
  - rubocop-rspec

inherit_gem:
  action_policy: config/rubocop-rspec.yml
```

```ruby
# spec/a_spec.rb
RSpec.describe 'A' do
  it_behaves_like 'a'
  it_behaves_like 'a'

  describe_rule :show? do
    succeed 'when post is published'
    succeed 'when post is published'
  end
end
```

Ideally, both the duplicated `it_behaves_like` and `succeed` should be
detected. However, `action_policy`'s `Includes/Examples` setting
overrides ours, and `it_behaves_like` disappears from this list. As a
result, `rubocop` only detects the duplication of `succeed`, but not of
`it_behaves_like`.
pirj added a commit to rubocop/rubocop-rspec that referenced this pull request Oct 3, 2021
fixes #1126

See palkan/action_policy#103 (comment)

If a third-party gem, e.g. `action_policy`, defines their
`config/default.yml` and supplements to the default RSpec DSL that we
provide in our `config/default.yml`, `rubocop`'s behaviour is to
actually override the configuration, as `inherit_mode` is `override` for
lists by default.

RuboCop has recently [fixed the problem with ignored
`inherit_mode`](rubocop/rubocop#9952) and we've
[bumped to use `rubocop` version that includes the
fix](#1181), but we haven't
adjusted our `config/default.yml` to merge by default.

This is causing both user project RSpec DSL configuration and
third-party gem RSpec DSL configuration to override our default,
rendering our cops blind.

### Example

A new project

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

gem 'action_policy'
gem 'rubocop-rspec'
```

```yml
# .rubocop.yml
require:
  - rubocop-rspec

inherit_gem:
  action_policy: config/rubocop-rspec.yml
```

```ruby
# spec/a_spec.rb
RSpec.describe 'A' do
  it_behaves_like 'a'
  it_behaves_like 'a'

  describe_rule :show? do
    succeed 'when post is published'
    succeed 'when post is published'
  end
end
```

Ideally, both the duplicated `it_behaves_like` and `succeed` should be
detected. However, `action_policy`'s `Includes/Examples` setting
overrides ours, and `it_behaves_like` disappears from this list. As a
result, `rubocop` only detects the duplication of `succeed`, but not of
`it_behaves_like`.
pirj added a commit to rubocop/rubocop-rspec that referenced this pull request Oct 5, 2021
fixes #1126

See palkan/action_policy#103 (comment)

If a third-party gem, e.g. `action_policy`, defines their
`config/default.yml` and supplements to the default RSpec DSL that we
provide in our `config/default.yml`, `rubocop`'s behaviour is to
actually override the configuration, as `inherit_mode` is `override` for
lists by default.

RuboCop has recently [fixed the problem with ignored
`inherit_mode`](rubocop/rubocop#9952) and we've
[bumped to use `rubocop` version that includes the
fix](#1181), but we haven't
adjusted our `config/default.yml` to merge by default.

This is causing both user project RSpec DSL configuration and
third-party gem RSpec DSL configuration to override our default,
rendering our cops blind.

### Example

A new project

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

gem 'action_policy'
gem 'rubocop-rspec'
```

```yml
# .rubocop.yml
require:
  - rubocop-rspec

inherit_gem:
  action_policy: config/rubocop-rspec.yml
```

```ruby
# spec/a_spec.rb
RSpec.describe 'A' do
  it_behaves_like 'a'
  it_behaves_like 'a'

  describe_rule :show? do
    succeed 'when post is published'
    succeed 'when post is published'
  end
end
```

Ideally, both the duplicated `it_behaves_like` and `succeed` should be
detected. However, `action_policy`'s `Includes/Examples` setting
overrides ours, and `it_behaves_like` disappears from this list. As a
result, `rubocop` only detects the duplication of `succeed`, but not of
`it_behaves_like`.
pirj added a commit to rubocop/rubocop-capybara that referenced this pull request Dec 29, 2022
pirj added a commit to rubocop/rubocop-capybara that referenced this pull request Dec 29, 2022
fixes #1126

See palkan/action_policy#103 (comment)

If a third-party gem, e.g. `action_policy`, defines their
`config/default.yml` and supplements to the default RSpec DSL that we
provide in our `config/default.yml`, `rubocop`'s behaviour is to
actually override the configuration, as `inherit_mode` is `override` for
lists by default.

RuboCop has recently [fixed the problem with ignored
`inherit_mode`](rubocop/rubocop#9952) and we've
[bumped to use `rubocop` version that includes the
fix](rubocop/rubocop-rspec#1181), but we haven't
adjusted our `config/default.yml` to merge by default.

This is causing both user project RSpec DSL configuration and
third-party gem RSpec DSL configuration to override our default,
rendering our cops blind.

### Example

A new project

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

gem 'action_policy'
gem 'rubocop-rspec'
```

```yml
# .rubocop.yml
require:
  - rubocop-rspec

inherit_gem:
  action_policy: config/rubocop-rspec.yml
```

```ruby
# spec/a_spec.rb
RSpec.describe 'A' do
  it_behaves_like 'a'
  it_behaves_like 'a'

  describe_rule :show? do
    succeed 'when post is published'
    succeed 'when post is published'
  end
end
```

Ideally, both the duplicated `it_behaves_like` and `succeed` should be
detected. However, `action_policy`'s `Includes/Examples` setting
overrides ours, and `it_behaves_like` disappears from this list. As a
result, `rubocop` only detects the duplication of `succeed`, but not of
`it_behaves_like`.
ydah pushed a commit to rubocop/rubocop-factory_bot that referenced this pull request Apr 13, 2023
ydah pushed a commit to rubocop/rubocop-factory_bot that referenced this pull request Apr 13, 2023
fixes #1126

See palkan/action_policy#103 (comment)

If a third-party gem, e.g. `action_policy`, defines their
`config/default.yml` and supplements to the default RSpec DSL that we
provide in our `config/default.yml`, `rubocop`'s behaviour is to
actually override the configuration, as `inherit_mode` is `override` for
lists by default.

RuboCop has recently [fixed the problem with ignored
`inherit_mode`](rubocop/rubocop#9952) and we've
[bumped to use `rubocop` version that includes the
fix](rubocop/rubocop-rspec#1181), but we haven't
adjusted our `config/default.yml` to merge by default.

This is causing both user project RSpec DSL configuration and
third-party gem RSpec DSL configuration to override our default,
rendering our cops blind.

### Example

A new project

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

gem 'action_policy'
gem 'rubocop-rspec'
```

```yml
# .rubocop.yml
require:
  - rubocop-rspec

inherit_gem:
  action_policy: config/rubocop-rspec.yml
```

```ruby
# spec/a_spec.rb
RSpec.describe 'A' do
  it_behaves_like 'a'
  it_behaves_like 'a'

  describe_rule :show? do
    succeed 'when post is published'
    succeed 'when post is published'
  end
end
```

Ideally, both the duplicated `it_behaves_like` and `succeed` should be
detected. However, `action_policy`'s `Includes/Examples` setting
overrides ours, and `it_behaves_like` disappears from this list. As a
result, `rubocop` only detects the duplication of `succeed`, but not of
`it_behaves_like`.
ydah pushed a commit to rubocop/rubocop-rspec_rails that referenced this pull request Mar 27, 2024
ydah pushed a commit to rubocop/rubocop-rspec_rails that referenced this pull request Mar 27, 2024
fixes #1126

See palkan/action_policy#103 (comment)

If a third-party gem, e.g. `action_policy`, defines their
`config/default.yml` and supplements to the default RSpec DSL that we
provide in our `config/default.yml`, `rubocop`'s behaviour is to
actually override the configuration, as `inherit_mode` is `override` for
lists by default.

RuboCop has recently [fixed the problem with ignored
`inherit_mode`](rubocop/rubocop#9952) and we've
[bumped to use `rubocop` version that includes the
fix](rubocop/rubocop-rspec#1181), but we haven't
adjusted our `config/default.yml` to merge by default.

This is causing both user project RSpec DSL configuration and
third-party gem RSpec DSL configuration to override our default,
rendering our cops blind.

### Example

A new project

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

gem 'action_policy'
gem 'rubocop-rspec'
```

```yml
# .rubocop.yml
require:
  - rubocop-rspec

inherit_gem:
  action_policy: config/rubocop-rspec.yml
```

```ruby
# spec/a_spec.rb
RSpec.describe 'A' do
  it_behaves_like 'a'
  it_behaves_like 'a'

  describe_rule :show? do
    succeed 'when post is published'
    succeed 'when post is published'
  end
end
```

Ideally, both the duplicated `it_behaves_like` and `succeed` should be
detected. However, `action_policy`'s `Includes/Examples` setting
overrides ours, and `it_behaves_like` disappears from this list. As a
result, `rubocop` only detects the duplication of `succeed`, but not of
`it_behaves_like`.
ydah pushed a commit to rubocop/rubocop-rspec_rails that referenced this pull request Mar 27, 2024
ydah pushed a commit to rubocop/rubocop-rspec_rails that referenced this pull request Mar 27, 2024
fixes #1126

See palkan/action_policy#103 (comment)

If a third-party gem, e.g. `action_policy`, defines their
`config/default.yml` and supplements to the default RSpec DSL that we
provide in our `config/default.yml`, `rubocop`'s behaviour is to
actually override the configuration, as `inherit_mode` is `override` for
lists by default.

RuboCop has recently [fixed the problem with ignored
`inherit_mode`](rubocop/rubocop#9952) and we've
[bumped to use `rubocop` version that includes the
fix](rubocop/rubocop-rspec#1181), but we haven't
adjusted our `config/default.yml` to merge by default.

This is causing both user project RSpec DSL configuration and
third-party gem RSpec DSL configuration to override our default,
rendering our cops blind.

### Example

A new project

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

gem 'action_policy'
gem 'rubocop-rspec'
```

```yml
# .rubocop.yml
require:
  - rubocop-rspec

inherit_gem:
  action_policy: config/rubocop-rspec.yml
```

```ruby
# spec/a_spec.rb
RSpec.describe 'A' do
  it_behaves_like 'a'
  it_behaves_like 'a'

  describe_rule :show? do
    succeed 'when post is published'
    succeed 'when post is published'
  end
end
```

Ideally, both the duplicated `it_behaves_like` and `succeed` should be
detected. However, `action_policy`'s `Includes/Examples` setting
overrides ours, and `it_behaves_like` disappears from this list. As a
result, `rubocop` only detects the duplication of `succeed`, but not of
`it_behaves_like`.
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

4 participants