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 default config for sub-departments #1163

Merged
merged 1 commit into from Oct 5, 2021

Conversation

pirj
Copy link
Member

@pirj pirj commented Jun 30, 2021

It turns out that the default config does not apply for sub-departments.
It is weird on many levels:

  • RSpec DSL configuration didn't work
  • if users would re-define RSpec DSL, they would have to do it for all sub-departments, as YAML defaults are only for the default config
  • if users would re-define RSpec DSL and use YAML defaults, too, the merging of the resulting config is undetermined

Still, I believe having Include to work correctly is the most important.

fixes #1160


Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • [-] Added tests.
  • [-] Updated documentation.
  • Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

@pirj pirj self-assigned this Jun 30, 2021
@pirj pirj force-pushed the fix-config-for-nested-departments branch from f9fdf16 to d3c48ae Compare June 30, 2021 16:13
@pirj
Copy link
Member Author

pirj commented Jun 30, 2021

@ShockwaveNN can you please check that this fix solves your case?

# Gemfile
gem 'rubocop-rspec', github: 'rubocop/rubocop-rspec', branch: 'fix-config-for-nested-departments'

@pirj pirj requested review from bquorning and Darhazer June 30, 2021 16:15
@pirj
Copy link
Member Author

pirj commented Jun 30, 2021

@jonas054 Looks good?

@pirj pirj force-pushed the fix-config-for-nested-departments branch from d3c48ae to 9e84186 Compare June 30, 2021 16:24
@ShockwaveNN
Copy link
Contributor

@pirj Can confirm, this do not broke my minitests

@pirj pirj force-pushed the fix-config-for-nested-departments branch from 9e84186 to b7e8995 Compare June 30, 2021 16:26
Copy link

@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.

Looks good as far as I can tell. 👍

@pirj
Copy link
Member Author

pirj commented Oct 5, 2021

Bump

Copy link
Member

@Darhazer Darhazer left a comment

Choose a reason for hiding this comment

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

I don't really understand the changes 🤷‍♂️ Please rebase anyway :)

@pirj pirj force-pushed the fix-config-for-nested-departments branch from b7e8995 to 880e01b Compare October 5, 2021 10:35
@pirj
Copy link
Member Author

pirj commented Oct 5, 2021

@Darhazer Rebased 👍

The explanation of the problem is in #1160 (comment)

configuration for the RSpec department isn't automatically inherited down to subdepartments like RSpec/Rails. I'm not sure if we could change RuboCop so that kind of inheritance takes place. The other solution would be to add

RSpec/Rails:
  Include:
    - "**/*_spec.rb"
    - "**/spec/**/*"

in config/default.yml of rubocop-rspec.

@@ -1,10 +1,10 @@
---
RSpec:
Enabled: true
Include:
Include: &1
Copy link
Member

Choose a reason for hiding this comment

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

It seems that this could be named, e.g. alphanumeric. Might make the file less scary, if it has proper names :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I vaguely recall I planned to do so (to mimic the well-known &default from config/database.yml 😄), but ended up with numbers somehow. Fixed 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, now I recall. It's bin/build_config that does that:

--- a/config/default.yml
+++ b/config/default.yml
@@ -1,10 +1,10 @@
 ---
 RSpec:
   Enabled: true
-  Include: &include
+  Include: &1
     - "**/*_spec.rb"
     - "**/spec/**/*"
-  Language: &language
+  Language: &2
     inherit_mode:
       merge:
         - Expectations

Supposedly, YAML.dump(unified_config) has no respect for named anchors.

I'll revert, otherwise CI is red.

@@ -24,6 +25,8 @@ def dump

def unified_config
cops.each_with_object(config.dup) do |cop, unified|
next if SUBDEPARTMENTS.include?(cop)
Copy link
Member

Choose a reason for hiding this comment

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

so those are not really cops, but departments declarations. Nice

Copy link
Member Author

Choose a reason for hiding this comment

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

Most of them are cops, but some are departments. It's because cops partially comes from config.keys. And we've included departments separately to config now.

@pirj pirj force-pushed the fix-config-for-nested-departments branch 2 times, most recently from a4f7a36 to 2e20ce1 Compare October 5, 2021 21:08
It turns out that the default config does not apply for sub-departments.
It is weird on many levels:
 - RSpec DSL configuration didn't work
 - - RSpec DSL doesn't work as intended anyway #1126
 - if users would re-define RSpec DSL, they would have to do it for all sub-departments, as YAML defaults are only for the default config
 - if users would re-define RSpec DSL and use YAML defaults, too, the merging of the resulting config is undetermined

Still, I believe having `Include` to work correctly is the most important.

fixes #1160
@pirj pirj force-pushed the fix-config-for-nested-departments branch from dc71418 to ed894a0 Compare October 5, 2021 21:25
@pirj pirj merged commit d832baa into master Oct 5, 2021
@pirj pirj deleted the fix-config-for-nested-departments branch October 5, 2021 21:30
@pirj pirj mentioned this pull request Oct 5, 2021
3 tasks
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.

RSpec/Rails/AvoidSetupHook consider minitest setup hook as problem
4 participants