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 AllowedPatterns configuration option to RSpec/ContextWording #1358

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* Fix incorrect documentation URLs when using `rubocop --show-docs-url`. ([@r7kamura][])
* Add `AllowedGroups` configuration option to `RSpec/NestedGroups`. ([@ydah][])
* Deprecate `IgnoredPatterns` option in favor of the `AllowedPatterns` options. ([@ydah][])
* Add `AllowedPatterns` configuration option to `RSpec/ContextWording`. ([@ydah][])

## 2.12.1 (2022-07-03)

Expand Down
3 changes: 2 additions & 1 deletion config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -212,8 +212,9 @@ RSpec/ContextWording:
- when
- with
- without
AllowedPatterns: []
Copy link
Member

Choose a reason for hiding this comment

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

Should we have default allowed patterns, matching the prefixes, and deprecate the prefixes option?

Copy link
Member Author

@ydah ydah Sep 1, 2022

Choose a reason for hiding this comment

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

I am certainly beginning to think that is a good.
@bquorning @pirj What do you think? I'd like to hear your opinion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can do that in a follow-up PR. We should also consider rewriting the cop documentation to mention AllowedPatterns before Prefixes.

VersionAdded: '1.20'
VersionChanged: 1.20.1
VersionChanged: '2.13'
StyleGuide: https://rspec.rubystyle.guide/#context-descriptions
Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/ContextWording

Expand Down
32 changes: 31 additions & 1 deletion docs/modules/ROOT/pages/cops_rspec.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,7 @@ end
| Yes
| No
| 1.20
| 1.20.1
| 2.13
|===

Checks that `context` docstring starts with an allowed prefix.
Expand All @@ -534,6 +534,9 @@ the configuration to meet project needs. Other acceptable prefixes may
include `if`, `unless`, `for`, `before`, `after`, or `during`.
They may consist of multiple words if desired.

This cop can be customized allowed context description pattern
with `AllowedPatterns`. By default, there are no checking by pattern.

=== Examples

==== `Prefixes` configuration
Expand Down Expand Up @@ -564,6 +567,29 @@ context 'when the display name is not present' do
end
----

==== `AllowedPatterns` configuration

[source,ruby]
----
# .rubocop.yml
# RSpec/ContextWording:
# AllowedPatterns:
# - /とき$/
----

[source,ruby]
----
# bad
context '条件を満たす' do
# ...
end
# good
context '条件を満たすとき' do
# ...
end
----

=== Configurable attributes

|===
Expand All @@ -572,6 +598,10 @@ end
| Prefixes
| `when`, `with`, `without`
| Array

| AllowedPatterns
| `[]`
| Array
|===

=== References
Expand Down
63 changes: 47 additions & 16 deletions lib/rubocop/cop/rspec/context_wording.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,41 +35,72 @@ module RSpec
# # ...
# end
#
# This cop can be customized allowed context description pattern
# with `AllowedPatterns`. By default, there are no checking by pattern.
#
# @example `AllowedPatterns` configuration
#
# # .rubocop.yml
# # RSpec/ContextWording:
# # AllowedPatterns:
# # - /とき$/
#
# @example
# # bad
# context '条件を満たす' do
# # ...
# end
#
# # good
# context '条件を満たすとき' do
# # ...
# end
#
class ContextWording < Base
MSG = 'Start context description with %<prefixes>s.'
include AllowedPattern

MSG = 'Context description should match %<patterns>s.'
Copy link
Member

Choose a reason for hiding this comment

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

I'm uncertain that I'm happy with this change.
From my perspective, the cognitive gap between "Start description with" and "Description should match /@$&#%&$$/" is noticeable.

Wondering if it's easy to use the old message when only Prefixes are defined, and the new one when AllowedPatterns are defined, too.
Or a mix of like "Context description should either start with % or match %".


# @!method context_wording(node)
def_node_matcher :context_wording, <<-PATTERN
(block (send #rspec? { :context :shared_context } $(str #bad_prefix?) ...) ...)
(block (send #rspec? { :context :shared_context } $(str $_) ...) ...)
PATTERN

def on_block(node) # rubocop:disable InternalAffairs/NumblockHandler
context_wording(node) do |context|
add_offense(context,
message: format(MSG, prefixes: joined_prefixes))
context_wording(node) do |context, description|
if bad_pattern?(description)
message = format(MSG, patterns: expect_patterns)
add_offense(context, message: message)
end
end
end

private

def bad_prefix?(description)
!prefix_regex.match?(description)
def allowed_patterns
super + prefix_regexes
end

def joined_prefixes
quoted = prefixes.map { |prefix| "'#{prefix}'" }
return quoted.first if quoted.size == 1
def prefix_regexes
@prefix_regexes ||= prefixes.map { |pre| /^#{Regexp.escape(pre)}\b/ }
end

def bad_pattern?(description)
return false if allowed_patterns.empty?

quoted << "or #{quoted.pop}"
quoted.join(', ')
!matches_allowed_pattern?(description)
end

def prefixes
cop_config['Prefixes'] || []
def expect_patterns
inspected = allowed_patterns.map(&:inspect)
return inspected.first if inspected.size == 1

inspected << "or #{inspected.pop}"
inspected.join(', ')
end

def prefix_regex
/^#{Regexp.union(prefixes)}\b/
def prefixes
Array(cop_config.fetch('Prefixes', []))
end
end
end
Expand Down
95 changes: 83 additions & 12 deletions spec/rubocop/cop/rspec/context_wording_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::RSpec::ContextWording do
let(:cop_config) { { 'Prefixes' => %w[when with] } }
let(:cop_config) do
{ 'Prefixes' => %w[when with without], 'AllowedPatterns' => [] }
end

it 'skips describe blocks' do
expect_no_offenses(<<-RUBY)
Expand All @@ -13,15 +15,15 @@
it 'finds context without `when` at the beginning' do
expect_offense(<<-RUBY)
context 'the display name not present' do
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Start context description with 'when', or 'with'.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.
end
RUBY
end

it 'finds shared_context without `when` at the beginning' do
expect_offense(<<-RUBY)
shared_context 'the display name not present' do
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Start context description with 'when', or 'with'.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.
end
RUBY
end
Expand All @@ -43,7 +45,7 @@
it 'finds context without separate `when` at the beginning' do
expect_offense(<<-RUBY)
context 'whenever you do' do
^^^^^^^^^^^^^^^^^ Start context description with 'when', or 'with'.
^^^^^^^^^^^^^^^^^ Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.
end
RUBY
end
Expand All @@ -52,7 +54,7 @@
it 'finds context without separate `when` at the beginning' do
expect_offense(<<-RUBY)
context 'whenever you do', legend: true do
^^^^^^^^^^^^^^^^^ Start context description with 'when', or 'with'.
^^^^^^^^^^^^^^^^^ Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.
end
RUBY
end
Expand All @@ -62,7 +64,7 @@
it 'finds context without separate `when` at the beginning' do
expect_offense(<<-RUBY)
context 'whenever you do', :legend do
^^^^^^^^^^^^^^^^^ Start context description with 'when', or 'with'.
^^^^^^^^^^^^^^^^^ Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.
end
RUBY
end
Expand All @@ -72,19 +74,19 @@
it 'finds context without separate `when` at the beginning' do
expect_offense(<<-RUBY)
context 'whenever you do', :legend, myth: true do
^^^^^^^^^^^^^^^^^ Start context description with 'when', or 'with'.
^^^^^^^^^^^^^^^^^ Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.
end
RUBY
end
end

context 'when configured' do
let(:cop_config) { { 'Prefixes' => %w[if] } }
let(:cop_config) { { 'Prefixes' => %w[if], 'AllowedPatterns' => [] } }

it 'finds context without allowed prefixes at the beginning' do
expect_offense(<<-RUBY)
context 'when display name is present' do
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Start context description with 'if'.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Context description should match /^if\\b/.
end
RUBY
end
Expand All @@ -97,7 +99,9 @@
end

context 'with a multi-word prefix' do
let(:cop_config) { { 'Prefixes' => ['assuming that'] } }
let(:cop_config) do
{ 'Prefixes' => ['assuming that'], 'AllowedPatterns' => [] }
end

it 'skips descriptions with allowed multi-word prefixes' do
expect_no_offenses(<<-RUBY)
Expand All @@ -108,12 +112,12 @@
end

context 'with special regex characters' do
let(:cop_config) { { 'Prefixes' => ['a$b\d'] } }
let(:cop_config) { { 'Prefixes' => ['a$b\d'], 'AllowedPatterns' => [] } }

it 'matches the full prefix' do
expect_offense(<<-RUBY)
context 'a' do
^^^ Start context description with 'a$b\\d'.
^^^ Context description should match /^a\\$b\\\\d\\b/.
end
RUBY
end
Expand All @@ -125,5 +129,72 @@
RUBY
end
end

context 'when `AllowedPatterns: [/とき$/]`' do
let(:cop_config) do
{ 'Prefixes' => [], 'AllowedPatterns' => [/とき$/] }
end

it 'finds context without `とき` at the ending' do
expect_offense(<<-RUBY)
context '条件を満たす' do
^^^^^^^^ Context description should match /とき$/.
end
RUBY
end

it 'finds shared_context without `とき` at the ending' do
expect_offense(<<-RUBY)
shared_context '条件を満たす' do
^^^^^^^^ Context description should match /とき$/.
end
RUBY
end

it "skips descriptions ending with 'とき'" do
expect_no_offenses(<<-RUBY)
context '条件を満たすとき' do
end
RUBY
end
end

context 'when `Prefixes: [when]` and `AllowedPatterns: [/patterns/]`' do
let(:cop_config) do
{ 'Prefixes' => %w[when], 'AllowedPatterns' => [/patterns/] }
end

it 'finds context without `when` at the beginning and not included ' \
'`/patterns/`' do
expect_offense(<<-RUBY)
context 'this is an incorrect context' do
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Context description should match /patterns/, or /^when\\b/.
end
RUBY
end

it 'finds shared_context without `when` at the beginning and ' \
'not included `/patterns/`' do
expect_offense(<<-RUBY)
shared_context 'this is an incorrect context' do
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Context description should match /patterns/, or /^when\\b/.
end
RUBY
end

it "skips descriptions beginning with 'when'" do
expect_no_offenses(<<-RUBY)
context 'when this is vaild context' do
end
RUBY
end

it "skips descriptions include with 'patterns'" do
expect_no_offenses(<<-RUBY)
context 'this is valid patterns context' do
end
RUBY
end
end
end
end