diff --git a/changelog/fix_glob_pattern_to_exclude_parent_dir.md b/changelog/fix_glob_pattern_to_exclude_parent_dir.md new file mode 100644 index 0000000000..e168877992 --- /dev/null +++ b/changelog/fix_glob_pattern_to_exclude_parent_dir.md @@ -0,0 +1 @@ +* [#1536](https://github.com/rubocop/rubocop-rails/issues/1536): Fix glob pattern. We added '**/' to support packwerk and engines, but this will glob directories above the project directory. Change to optional '../**/' ([@malmckay][]) diff --git a/config/default.yml b/config/default.yml index 65568b6913..b97af74793 100644 --- a/config/default.yml +++ b/config/default.yml @@ -6,7 +6,7 @@ inherit_mode: AllCops: Exclude: - - '**/app/assets/**/*' + - '{../**/,}app/assets/**/*' - bin/* # Exclude db/schema.rb and db/[CONFIGURATION_NAMESPACE]_schema.rb by default. # See: https://guides.rubyonrails.org/active_record_multiple_databases.html#setting-up-your-application @@ -94,8 +94,8 @@ Lint/UselessAccessModifier: Lint/UselessMethodDefinition: # Avoids conflict with `Rails/LexicallyScopedActionFilter` cop. Exclude: - - '**/app/controllers/**/*.rb' - - '**/app/mailers/**/*.rb' + - '{../**/,}app/controllers/**/*.rb' + - '{../**/,}app/mailers/**/*.rb' Rails: Enabled: true @@ -127,8 +127,8 @@ Rails/ActionFilter: - action - filter Include: - - '**/app/controllers/**/*.rb' - - '**/app/mailers/**/*.rb' + - '{../**/,}app/controllers/**/*.rb' + - '{../**/,}app/mailers/**/*.rb' Rails/ActionOrder: Description: 'Enforce consistent ordering of controller actions.' @@ -143,7 +143,7 @@ Rails/ActionOrder: - update - destroy Include: - - '**/app/controllers/**/*.rb' + - '{../**/,}app/controllers/**/*.rb' Rails/ActiveRecordAliases: Description: >- @@ -160,7 +160,7 @@ Rails/ActiveRecordCallbacksOrder: Enabled: 'pending' VersionAdded: '2.7' Include: - - '**/app/models/**/*.rb' + - '{../**/,}app/models/**/*.rb' Rails/ActiveRecordOverride: Description: >- @@ -171,7 +171,7 @@ Rails/ActiveRecordOverride: VersionAdded: '0.67' VersionChanged: '2.18' Include: - - '**/app/models/**/*.rb' + - '{../**/,}app/models/**/*.rb' Rails/ActiveSupportAliases: Description: >- @@ -258,7 +258,7 @@ Rails/AttributeDefaultBlockValue: Enabled: pending VersionAdded: '2.9' Include: - - '**/app/models/**/*' + - '{../**/,}app/models/**/*' Rails/BelongsTo: Description: >- @@ -317,8 +317,8 @@ Rails/ContentTag: # https://puma.io/puma/Puma/DSL.html#tag-instance_method # No helpers are used in normal models and configs. Exclude: - - '**/app/models/**/*.rb' - - '**/config/**/*.rb' + - '{../**/,}app/models/**/*.rb' + - '{../**/,}config/**/*.rb' Rails/CreateTableWithTimestamps: Description: >- @@ -379,7 +379,7 @@ Rails/Delegate: # violation. When set to false, this case is legal. EnforceForPrefixed: true Exclude: - - '**/app/controllers/**/*.rb' + - '{../**/,}app/controllers/**/*.rb' Rails/DelegateAllowBlank: Description: 'Do not use allow_blank as an option to delegate.' @@ -450,7 +450,7 @@ Rails/EnumHash: Enabled: true VersionAdded: '2.3' Include: - - '**/app/models/**/*.rb' + - '{../**/,}app/models/**/*.rb' Rails/EnumSyntax: Description: 'Use positional arguments over keyword arguments when defining enums.' @@ -458,15 +458,15 @@ Rails/EnumSyntax: Severity: warning VersionAdded: '2.26' Include: - - '**/app/models/**/*.rb' - - '**/lib/**/*.rb' + - '{../**/,}app/models/**/*.rb' + - '{../**/,}lib/**/*.rb' Rails/EnumUniqueness: Description: 'Avoid duplicate integers in hash-syntax `enum` declaration.' Enabled: true VersionAdded: '0.46' Include: - - '**/app/models/**/*.rb' + - '{../**/,}app/models/**/*.rb' Rails/Env: Description: 'Use Feature Flags or config instead of `Rails.env`.' @@ -490,11 +490,11 @@ Rails/EnvironmentVariableAccess: VersionAdded: '2.10' VersionChanged: '2.24' Include: - - '**/app/**/*.rb' - - '**/config/initializers/**/*.rb' - - '**/lib/**/*.rb' + - '{../**/,}app/**/*.rb' + - '{../**/,}config/initializers/**/*.rb' + - '{../**/,}lib/**/*.rb' Exclude: - - '**/lib/**/*.rake' + - '{../**/,}lib/**/*.rake' AllowReads: false AllowWrites: false @@ -506,11 +506,11 @@ Rails/Exit: Enabled: true VersionAdded: '0.41' Include: - - '**/app/**/*.rb' - - '**/config/**/*.rb' - - '**/lib/**/*.rb' + - '{../**/,}app/**/*.rb' + - '{../**/,}config/**/*.rb' + - '{../**/,}lib/**/*.rb' Exclude: - - '**/lib/**/*.rake' + - '{../**/,}lib/**/*.rake' Rails/ExpandedDateRange: Description: 'Checks for expanded date range.' @@ -579,7 +579,7 @@ Rails/HasAndBelongsToMany: Enabled: true VersionAdded: '0.12' Include: - - '**/app/models/**/*.rb' + - '{../**/,}app/models/**/*.rb' Rails/HasManyOrHasOneDependent: Description: 'Define the dependent option to the has_many and has_one associations.' @@ -587,14 +587,14 @@ Rails/HasManyOrHasOneDependent: Enabled: true VersionAdded: '0.50' Include: - - '**/app/models/**/*.rb' + - '{../**/,}app/models/**/*.rb' Rails/HelperInstanceVariable: Description: 'Do not use instance variables in helpers.' Enabled: true VersionAdded: '2.0' Include: - - '**/app/helpers/**/*.rb' + - '{../**/,}app/helpers/**/*.rb' Rails/HttpPositionalArguments: Description: 'Use keyword arguments instead of positional arguments in http method calls.' @@ -620,7 +620,7 @@ Rails/HttpStatusNameConsistency: Severity: warning VersionAdded: '<>' Include: - - '**/app/controllers/**/*.rb' + - '{../**/,}app/controllers/**/*.rb' Rails/I18nLazyLookup: Description: 'Checks for places where I18n "lazy" lookup can be used.' @@ -633,7 +633,7 @@ Rails/I18nLazyLookup: - lazy - explicit Include: - - '**/app/controllers/**/*.rb' + - '{../**/,}app/controllers/**/*.rb' Rails/I18nLocaleAssignment: Description: 'Prefer the usage of `I18n.with_locale` instead of manually updating `I18n.locale` value.' @@ -662,8 +662,8 @@ Rails/IgnoredSkipActionFilterOption: Enabled: true VersionAdded: '0.63' Include: - - '**/app/controllers/**/*.rb' - - '**/app/mailers/**/*.rb' + - '{../**/,}app/controllers/**/*.rb' + - '{../**/,}app/mailers/**/*.rb' Rails/IndexBy: Description: 'Prefer `index_by` over `each_with_object`, `to_h`, or `map`.' @@ -693,7 +693,7 @@ Rails/InverseOf: VersionAdded: '0.52' IgnoreScopes: false Include: - - '**/app/models/**/*.rb' + - '{../**/,}app/models/**/*.rb' Rails/LexicallyScopedActionFilter: Description: "Checks that methods specified in the filter's `only` or `except` options are explicitly defined in the class." @@ -702,8 +702,8 @@ Rails/LexicallyScopedActionFilter: Safe: false VersionAdded: '0.52' Include: - - '**/app/controllers/**/*.rb' - - '**/app/mailers/**/*.rb' + - '{../**/,}app/controllers/**/*.rb' + - '{../**/,}app/mailers/**/*.rb' Rails/LinkToBlank: Description: 'Checks that `link_to` with a `target: "_blank"` have a `rel: "noopener"` option passed to them.' @@ -721,7 +721,7 @@ Rails/MailerName: SafeAutoCorrect: false VersionAdded: '2.7' Include: - - '**/app/mailers/**/*.rb' + - '{../**/,}app/mailers/**/*.rb' Rails/MatchRoute: Description: >- @@ -731,8 +731,8 @@ Rails/MatchRoute: Enabled: 'pending' VersionAdded: '2.7' Include: - - '**/config/routes.rb' - - '**/config/routes/**/*.rb' + - '{../**/,}config/routes.rb' + - '{../**/,}config/routes/**/*.rb' Rails/MigrationClassName: Description: 'The class name of the migration should match its file name.' @@ -748,8 +748,8 @@ Rails/MultipleRoutePaths: Severity: warning VersionAdded: '2.29' Include: - - '**/config/routes.rb' - - '**/config/routes/**/*.rb' + - '{../**/,}config/routes.rb' + - '{../**/,}config/routes/**/*.rb' Rails/NegateInclude: Description: 'Prefer `collection.exclude?(obj)` over `!collection.include?(obj)`.' @@ -792,10 +792,10 @@ Rails/Output: VersionAdded: '0.15' VersionChanged: '0.19' Include: - - '**/app/**/*.rb' - - '**/config/**/*.rb' + - '{../**/,}app/**/*.rb' + - '{../**/,}config/**/*.rb' - db/**/*.rb - - '**/lib/**/*.rb' + - '{../**/,}lib/**/*.rb' Rails/OutputSafety: Description: 'The use of `html_safe` or `raw` may be a security risk.' @@ -868,7 +868,7 @@ Rails/RakeEnvironment: - '**/Rakefile' - '**/*.rake' Exclude: - - '**/lib/capistrano/tasks/**/*.rake' + - '{../**/,}lib/capistrano/tasks/**/*.rake' Rails/ReadWriteAttribute: Description: >- @@ -879,7 +879,7 @@ Rails/ReadWriteAttribute: VersionAdded: '0.20' VersionChanged: '0.29' Include: - - '**/app/models/**/*.rb' + - '{../**/,}app/models/**/*.rb' Rails/RedirectBackOrTo: Description: >- @@ -906,7 +906,7 @@ Rails/RedundantAllowNil: Enabled: true VersionAdded: '0.67' Include: - - '**/app/models/**/*.rb' + - '{../**/,}app/models/**/*.rb' Rails/RedundantForeignKey: Description: 'Checks for associations where the `:foreign_key` option is redundant.' @@ -1077,7 +1077,7 @@ Rails/ScopeArgs: VersionAdded: '0.19' VersionChanged: '2.12' Include: - - '**/app/models/**/*.rb' + - '{../**/,}app/models/**/*.rb' Rails/SelectMap: Description: 'Checks for uses of `select(:column_name)` with `map(&:column_name)`.' @@ -1146,7 +1146,7 @@ Rails/StrongParametersExpect: Reference: 'https://api.rubyonrails.org/classes/ActionController/Parameters.html#method-i-expect' Enabled: pending Include: - - '**/app/controllers/**/*.rb' + - '{../**/,}app/controllers/**/*.rb' SafeAutoCorrect: false VersionAdded: '2.29' @@ -1157,7 +1157,7 @@ Rails/TableNameAssignment: Enabled: false VersionAdded: '2.14' Include: - - '**/app/models/**/*.rb' + - '{../**/,}app/models/**/*.rb' Rails/ThreeStateBooleanColumn: Description: 'Add a default value and a `NOT NULL` constraint to boolean columns.' @@ -1241,7 +1241,7 @@ Rails/UniqueValidationWithoutIndex: Enabled: true VersionAdded: '2.5' Include: - - '**/app/models/**/*.rb' + - '{../**/,}app/models/**/*.rb' Rails/UnknownEnv: Description: 'Use correct environment name.' @@ -1260,7 +1260,7 @@ Rails/UnusedIgnoredColumns: VersionAdded: '2.11' VersionChanged: '2.25' Include: - - '**/app/models/**/*.rb' + - '{../**/,}app/models/**/*.rb' Rails/UnusedRenderContent: Description: 'Do not specify body content for a response with a non-content status code.' @@ -1274,7 +1274,7 @@ Rails/Validation: VersionAdded: '0.9' VersionChanged: '0.41' Include: - - '**/app/models/**/*.rb' + - '{../**/,}app/models/**/*.rb' Rails/WhereEquals: Description: 'Pass conditions to `where` and `where.not` as a hash instead of manually constructing SQL.' diff --git a/spec/project_spec.rb b/spec/project_spec.rb index f9466d6f00..30f1637a9b 100644 --- a/spec/project_spec.rb +++ b/spec/project_spec.rb @@ -158,15 +158,10 @@ next if config[name][clusivity_key].nil? config[name][clusivity_key].each do |clusivity_pattern| - expect(clusivity_pattern).to match(%r{\*\*/app/}), <<~ERROR if clusivity_pattern.match?(%r{\bapp/}) - Invalid pattern for #{name} #{clusivity_key}: #{clusivity_pattern} - ERROR - expect(clusivity_pattern).to match(%r{\*\*/config/}), <<~ERROR if clusivity_pattern.match?(%r{\bconfig/}) - Invalid pattern for #{name} #{clusivity_key}: #{clusivity_pattern} - ERROR - expect(clusivity_pattern).to match(%r{\*\*/lib/}), <<~ERROR if clusivity_pattern.match?(%r{\blib/}) - Invalid pattern for #{name} #{clusivity_key}: #{clusivity_pattern} - ERROR + msg = "Invalid pattern for #{name} #{clusivity_key}: #{clusivity_pattern}" + expect(clusivity_pattern).to match(%r[{../\*\*/,}app/]), msg if clusivity_pattern.match?(%r{\bapp/}) + expect(clusivity_pattern).to match(%r[{../\*\*/,}config/]), msg if clusivity_pattern.match?(%r{\bconfig/}) + expect(clusivity_pattern).to match(%r[{../\*\*/,}lib/]), msg if clusivity_pattern.match?(%r{\blib/}) end end end diff --git a/spec/rubocop/cli/file_glob_spec.rb b/spec/rubocop/cli/file_glob_spec.rb new file mode 100644 index 0000000000..65263208b3 --- /dev/null +++ b/spec/rubocop/cli/file_glob_spec.rb @@ -0,0 +1,93 @@ +# frozen_string_literal: true + +# We use ../**/ to match standard Rails directories for Include and Exclude patterns +# ** is necessary to match within packwerk and engine directories. +# .. is necessary to exclude the top level directory (if your project is in /var/app/my_project, for example) +# Rubocop::PathUtil has special handling for .. that makes this work +RSpec.describe 'RuboCop::CLI', :isolated_environment do + subject(:cli) { RuboCop::CLI.new } + + include_context 'cli spec behavior' + + before do + RuboCop::ConfigLoader.default_configuration.for_all_cops['SuggestExtensions'] = false + end + + it 'detects offense when Include matches the path' do + create_file('.rubocop.yml', <<~YAML) + Rails/EnvironmentVariableAccess: + Enabled: true + Include: + - '../**/app/**/*.rb' + YAML + + create_file('app/example.rb', <<~RUBY) + ENV['RUBY_ENV'] = 'do not set environment variables directly!' + RUBY + + expect(cli.run(['--only', 'Rails/EnvironmentVariableAccess'])).to eq(1) + expect($stdout.string).to match('1 offense detected') + end + + it 'detects offense when Include matches the path when nested in a subdirectory' do + create_file('.rubocop.yml', <<~YAML) + Rails/EnvironmentVariableAccess: + Enabled: true + Include: + - '../**/app/**/*.rb' + YAML + + create_file('packwerk/app/example.rb', <<~RUBY) + ENV['RUBY_ENV'] = 'do not set environment variables directly!' + RUBY + + expect(cli.run(['--only', 'Rails/EnvironmentVariableAccess'])).to eq(1) + expect($stdout.string).to match('1 offense detected') + end + + it 'does not detect offense when Include does not match the path' do + create_file('.rubocop.yml', <<~YAML) + Rails/EnvironmentVariableAccess: + Enabled: true + Include: + - '../**/app/**/*.rb' + YAML + + create_file('example.rb', <<~RUBY) + ENV['RUBY_ENV'] = 'do not set environment variables directly!' + RUBY + expect(cli.run(['--only', 'Rails/EnvironmentVariableAccess'])).to eq(0) + end + + it 'does detect offense when the root directory is also in Include' do + top_level_dir = Pathname.new(Dir.pwd).each_filename.first + + create_file('.rubocop.yml', <<~YAML) + Rails/EnvironmentVariableAccess: + Enabled: true + Include: + - '../**/#{top_level_dir}/**/*.rb' + YAML + + create_file(File.join(top_level_dir, 'example.rb'), <<~RUBY) + ENV['RUBY_ENV'] = 'do not set environment variables directly!' + RUBY + expect(cli.run(['--only', 'Rails/EnvironmentVariableAccess'])).to eq(1) + end + + it 'does not detect offense when the root directory is also in Include, but not in the file path' do + top_level_dir = Pathname.new(Dir.pwd).each_filename.first + + create_file('.rubocop.yml', <<~YAML) + Rails/EnvironmentVariableAccess: + Enabled: true + Include: + - '../**/#{top_level_dir}/**/*.rb' + YAML + + create_file(File.join('example.rb'), <<~RUBY) + ENV['RUBY_ENV'] = 'do not set environment variables directly!' + RUBY + expect(cli.run(['--only', 'Rails/EnvironmentVariableAccess'])).to eq(0) + end +end