Add `AllowedPatterns` configuration option to `RSpec/ContextWording` #1358

ydah · 2022-08-05T11:36:16Z

Resolve: #1356

This PR is add the AllowedPattern option.

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

If you have created a new cop:

[-] Added the new cop to config/default.yml.
[-] The cop is configured as Enabled: pending in config/default.yml.
[-] The cop is configured as Enabled: true in .rubocop.yml.
[-] The cop documents examples of good and bad code.
[-] The tests assert both that bad code is reported and that good code is not reported.
[-] Set VersionAdded in default/config.yml to the next minor version.

If you have modified an existing cop's configuration options:

Set VersionChanged in config/default.yml to the next major version.

pirj · 2022-08-09T20:00:41Z

docs/modules/ROOT/pages/cops_rspec.adoc

+end
+
+# good
+context 'when this test is success' do


I'd really prefer this to be in an RTL language, as with LTR it's hard to imagine anyone willing to constrain descriptions to be suffixed with a limited set of words.

I agree, an RTL language example would work much better here.

If it is an RTL language, it appears that if /olloeh$/ is specified, it will correctly check if it ends in olloeh as an RTL language.

# bad context '!sknaht olleh' end # good context 'olleh !sknaht' end

How about using Japanese as an example, as in the case of the original Issue?

bquorning · 2022-08-12T08:48:11Z

Should we consider changing the cop to be configured with regular expressions instead of prefixes and suffixes? Then people can use them however they like. The current defaults would need to be changed to e.g.

RSpec/ContextWording:
  Prefixes:
    - "^when\b"
    - "^with\b"
    - "^without\b"

ydah · 2022-08-15T22:32:08Z

Certainly we might consider changing the cop to be composed of regular expressions.

Would it be better to allow regular expressions to be set with names like AllowedPattern instead of the option names Prefixes and Suffixes?
In that case, it seems to me that it would be better to make Prefixes obsoleted but not removed, and migrate slowly while ensuring backward compatibility.

pirj · 2022-08-16T07:09:27Z

I'd keep Prefixes and introduced a AllowedPatterns to support both suffixes and prefixes. WDYT?

ydah · 2022-08-16T08:35:05Z

The following PR is needed:

Fix InternalAffairs/NumblockHandler offenses #1362

ydah · 2022-08-30T10:50:41Z

Conflicts have been resolved.

bquorning

I think we also need a spec for when Prefixes is empty, but AllowedPatterns is not.

bquorning · 2022-08-31T06:34:07Z

docs/modules/ROOT/pages/cops_rspec.adoc

+end
+
+# good
+context 'when this test is success' do


I agree, an RTL language example would work much better here.

spec/rubocop/cop/rspec/context_wording_spec.rb

bquorning

I am sorry for stalling this pull request, but there are still a few things I would like to change 😅

I think we should stop treating the Prefixes as anything special. I think if we override the #allowed_patterns method to include the prefixes, we can get rid of PREFIX_MSG, BOTH_MSG and a bunch of methods. E.g.

def allowed_patterns
  @allowed_patterns ||= begin
    prefix_regexes = prefixes.map { |pre| /^#{Regexp.escape(pre)}\b/ }
    super.concat(prefix_regexes)
  end
end

(Without the memoization it seems that the list grows unbounded. I didn’t have time to check why that happens)

Another thing to be aware of is that “#inspect actually produces the more natural version of the [Regex] string than #to_s”. So, by using "'#{value.inspect}'" instead of "'#{value}'", we can have offense messages with '/とき$/' instead of '(?-mix:とき$)'. (That's also why I chose to map the Regexp.escaped prefixes – that way they look better in offense messages).

This is of course up for discussion. I’m looking forward to hearing your opinion.

ydah · 2022-08-31T22:23:31Z

I am sorry for stalling this pull request, but there are still a few things I would like to change 😅

If it makes it better, even if it takes longer, it's worth it ;-)
I sincerely appreciate your always thoughtful reviews. @bquorning @Darhazer @pirj

The approach of overriding the #allowed_patterns method without special attention to Prefixes seems so good.
It also keeps the code very simple and seems to improve the outlook of the ContextWording class, which was a bit bloated.
This would also unify the messages and make judging very simple.

I also think it would be a good idea to use Object#inspect in the violation message.
In that case, there is no need to further quoted string like a #{"'v.inspect'"}, so I put the result of Object#inspect directly into the offense message, what do you think?

lib/rubocop/cop/rspec/context_wording.rb

Changes were applied

bquorning

Thank you so much @ydah 🙏🏼

Darhazer · 2022-09-01T10:40:14Z

config/default.yml

@@ -212,8 +212,9 @@ RSpec/ContextWording:
    - when
    - with
    - without
+  AllowedPatterns: []


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

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

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

Darhazer · 2022-09-01T10:43:02Z

spec/rubocop/cop/rspec/context_wording_spec.rb

@@ -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/, or /^with\\b/.


with love to see an example with 3 patterns

Indeed it is. I updated it:

rubocop-rspec/spec/rubocop/cop/rspec/context_wording_spec.rb

Lines 15 to 21 in c017ffd

it 'finds context without `when` at the beginning' do

expect_offense(<<-RUBY)

context 'the display name not present' do

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.

end

RUBY

end

Resolve: rubocop#1356

pirj

Happy to see the cop works for RTL now, too.
Just a small optional subjective note, maybe an idea for a follow-up if it gets some support.

pirj · 2022-09-01T18:38:09Z

lib/rubocop/cop/rspec/context_wording.rb

-        MSG = 'Start context description with %<prefixes>s.'
+        include AllowedPattern
+
+        MSG = 'Context description should match %<patterns>s.'


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 %".

ydah force-pushed the feature/suffix_support_context_wording branch from 7fa2a1c to 25ed74f Compare August 5, 2022 11:36

pirj reviewed Aug 9, 2022

View reviewed changes

ydah force-pushed the feature/suffix_support_context_wording branch 2 times, most recently from 7e390b8 to 6fd5d9b Compare August 16, 2022 08:00

ydah changed the title ~~Add Suffixes configuration option to RSpec/ContextWording~~ Add AllowedPatterns configuration option to RSpec/ContextWording Aug 16, 2022

ydah force-pushed the feature/suffix_support_context_wording branch 2 times, most recently from e496c23 to c6a0b8d Compare August 25, 2022 11:37

ydah requested a review from pirj August 25, 2022 11:40

pirj requested review from Darhazer and bquorning and removed request for pirj August 26, 2022 08:05

ydah force-pushed the feature/suffix_support_context_wording branch from c6a0b8d to 9f7cdd7 Compare August 30, 2022 10:50

bquorning previously requested changes Aug 31, 2022

View reviewed changes

ydah force-pushed the feature/suffix_support_context_wording branch 2 times, most recently from 67125e3 to b6ba8de Compare August 31, 2022 08:51

ydah commented Aug 31, 2022

View reviewed changes

spec/rubocop/cop/rspec/context_wording_spec.rb Outdated Show resolved Hide resolved

bquorning reviewed Aug 31, 2022

View reviewed changes

ydah force-pushed the feature/suffix_support_context_wording branch from b6ba8de to d7c629a Compare August 31, 2022 22:08

ydah requested review from bquorning and pirj and removed request for Darhazer, bquorning and pirj August 31, 2022 22:47

bquorning reviewed Sep 1, 2022

View reviewed changes

lib/rubocop/cop/rspec/context_wording.rb Outdated Show resolved Hide resolved

ydah force-pushed the feature/suffix_support_context_wording branch 2 times, most recently from 1030731 to 8ea4ae0 Compare September 1, 2022 08:00

bquorning requested a review from pirj September 1, 2022 08:34

bquorning approved these changes Sep 1, 2022

View reviewed changes

ydah force-pushed the feature/suffix_support_context_wording branch from 8ea4ae0 to 1efcaae Compare September 1, 2022 10:42

Darhazer approved these changes Sep 1, 2022

View reviewed changes

Add AllowedPatterns configuration option to RSpec/ContextWording

c017ffd

Resolve: rubocop#1356

ydah force-pushed the feature/suffix_support_context_wording branch from 1efcaae to c017ffd Compare September 1, 2022 10:55

Darhazer merged commit bfaa66e into rubocop:master Sep 1, 2022

pirj reviewed Sep 1, 2022

View reviewed changes

ydah deleted the feature/suffix_support_context_wording branch January 23, 2023 07:16

ydah mentioned this pull request Jan 23, 2023

Feature request: RSpec/ContextWording supports Japanese #745

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `AllowedPatterns` configuration option to `RSpec/ContextWording` #1358

Add `AllowedPatterns` configuration option to `RSpec/ContextWording` #1358

ydah commented Aug 5, 2022 •

edited

pirj Aug 9, 2022

bquorning Aug 31, 2022

ydah Aug 31, 2022

bquorning commented Aug 12, 2022

ydah commented Aug 15, 2022

pirj commented Aug 16, 2022

ydah commented Aug 16, 2022

ydah commented Aug 30, 2022

bquorning left a comment

bquorning Aug 31, 2022

bquorning left a comment

ydah commented Aug 31, 2022 •

edited

bquorning left a comment

Darhazer Sep 1, 2022

ydah Sep 1, 2022 •

edited

bquorning Sep 1, 2022

Darhazer Sep 1, 2022

ydah Sep 1, 2022 •

edited

Darhazer Sep 1, 2022

pirj left a comment

pirj Sep 1, 2022

	it 'finds context without `when` at the beginning' do
	expect_offense(<<-RUBY)
	context 'the display name not present' do
	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Context description should match /^when\\b/, /^with\\b/, or /^without\\b/.
	end
	RUBY
	end

Add AllowedPatterns configuration option to RSpec/ContextWording #1358

Add AllowedPatterns configuration option to RSpec/ContextWording #1358

Conversation

ydah commented Aug 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bquorning commented Aug 12, 2022

ydah commented Aug 15, 2022

pirj commented Aug 16, 2022

ydah commented Aug 16, 2022

ydah commented Aug 30, 2022

bquorning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bquorning left a comment

Choose a reason for hiding this comment

ydah commented Aug 31, 2022 • edited

bquorning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ydah Sep 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ydah Sep 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `AllowedPatterns` configuration option to `RSpec/ContextWording` #1358

Add `AllowedPatterns` configuration option to `RSpec/ContextWording` #1358

ydah commented Aug 5, 2022 •

edited

ydah commented Aug 31, 2022 •

edited

ydah Sep 1, 2022 •

edited

ydah Sep 1, 2022 •

edited