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 TargetRailsVersion to default.yml #11833

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

searls
Copy link

@searls searls commented May 1, 2023

After discovering that a valid configuration can't be created programmatically if it includes TargetRailsVersion without patching it in by doing an instance_variable_set on the ConfigLoader's @default_configuration, it seems appropriate to explicitly list this property even though it's specific to rubocop-rails.

For example, before this commit, running:

RuboCop::Config.create({
  "AllCops" => {
    "TargetRailsVersion" => 5.2
  }
}, "some_path")

Will emit this warning:

Warning: AllCops does not support TargetRailsVersion parameter.

Supported parameters are:

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

Even though this is the domain of a plugin, it seems appropriate to add it since:

  1. ActiveSupportExtensionsEnabled has similarly been added (perhaps for the same reason?)
  2. RuboCop itself (and not rubocop-rails) actually contains the logic handling this setting in Config

Replace this text with a summary of the changes in your PR.
The more detailed you are, the better.


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.

After discovering that a valid configuration can't be created programmatically if it includes `TargetRailsVersion` without [patching it in](https://github.com/rubocop/rubocop-rails/blob/master/lib/rubocop/rails/inject.rb#L14) by doing an `instance_variable_set` on the `ConfigLoader`'s `@default_configuration`, it seems appropriate to explicitly list this property even though it's specific to rubocop-rails.

For example, before this commit, running: 

```ruby
RuboCop::Config.create({
  "AllCops" => {
    "TargetRailsVersion" => 5.2
  }
}, "some_path")
```

Will emit this warning:

```
Warning: AllCops does not support TargetRailsVersion parameter.

Supported parameters are:

  - RubyInterpreters
  - Include
  - Exclude
  - DefaultFormatter
  - DisplayCopNames
  - DisplayStyleGuide
  - StyleGuideBaseURL
  - DocumentationBaseURL
  - ExtraDetails
  - StyleGuideCopsOnly
  - EnabledByDefault
  - DisabledByDefault
  - NewCops
  - UseCache
  - MaxFilesInCache
  - CacheRootDirectory
  - AllowSymlinksInCacheRootDirectory
  - TargetRubyVersion
  - SuggestExtensions
  - ActiveSupportExtensionsEnabled
```

Even though this is the domain of a plugin, it seems appropriate to add it since:

1. `ActiveSupportExtensionsEnabled` has similarly been added (perhaps for the same reason?)
2. RuboCop itself (and not rubocop-rails) actually contains the logic handling this setting  in [Config](/lib/rubocop/config.rb#L249-L258)
@koic
Copy link
Member

koic commented May 2, 2023

This is a tricky part.

  1. ActiveSupportExtensionsEnabled has similarly been added (perhaps for the same reason?)

This option has been introduced for implementation convenience. For example RuboCop Rails sets ActiveSupportExtensionsEnabled: true. It's not positive, but it's a realistic solution for now. Recently suggested: rubocop/rubocop-performance#342 (comment)

RuboCop itself (and not rubocop-rails) actually contains the logic handling this setting in Config

Code around TargetRailsVersion should be extracted to RuboCop Rails, but no progress yet. Either way it would be expected to be removed from RuboCop core in future.

I'm still not sure if it's best to restore the TargetRailsVersion to the core.

@searls
Copy link
Author

searls commented May 2, 2023

Agree that this is tricky! The division is not very clear right now which makes it a little hard to work with the API.

I was able to work around these issues for standard-rails yesterday, so there's no urgency in my request

@bbatsov
Copy link
Collaborator

bbatsov commented Jun 20, 2023

Code around TargetRailsVersion should be extracted to RuboCop Rails, but no progress yet. Either way it would be expected to be removed from RuboCop core in future.

Agreed.

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

3 participants