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.

Sign up for GitHub

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

Complete branch coverage for ContextWording #1972

Merged
merged 1 commit into from
Oct 21, 2024
+13 −0
Merged

Complete branch coverage for ContextWording #1972

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions spec/rubocop/cop/rspec/context_wording_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,4 +214,17 @@
end
end
end

context 'when `AllowedPatterns:` and `Prefixes:` are both empty' do
Copy link
Member

Choose a reason for hiding this comment

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

memo) In this case, it is pointless to enable this cop because it is not always an offense. Should we occur warning or runtime error for this setting? WDYT? @rubocop/rubocop-rspec

Copy link
Collaborator

Choose a reason for hiding this comment

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

it is not always an offense

I don’t understand. Do you mean it’s not deterministic?

Copy link
Member

@ydah ydah Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The following checks are always considered non-offenses:

rubocop-rspec/lib/rubocop/cop/rspec/context_wording.rb

Line 88 in 7592cec

return false if allowed_patterns.empty?

This Cop sets valid wording in Prefixes and AllowedPatterns, but it always dose not occur offenses if there is no valid wording setting. I don't think that's the intended setting, but users have no way of knowing right now. Because it's always dose not occur offenses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, with these settings, there will be no offenses, regardless of what is or is not in the Context. It is always not an offense. (I'm not attached to it staying that way).

I looked at the Obsoletion setup recently, and I think it raises runtime errors with the expectation that the user will want to fix those issues. That feels similar to how a user may want to fix the config or disable this cop instead of letting it run "on empty"

Copy link
Collaborator

Choose a reason for hiding this comment

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

It’s an unlikely combination of settings, so unless it’s very simple to implement, I don‘t think we need to warn our users about it.

Copy link
Member

@ydah ydah Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If the Prefixes and AllowedPatterns are empty to begin with, there is no allowed Wording, so it might be better to make them all violations. That way, users would notice the misconfiguration and it would not be too difficult to implement.

let(:cop_config) do
{ 'Prefixes' => [], 'AllowedPatterns' => [] }
end

it 'skips any description' do
expect_no_offenses(<<~RUBY)
context 'arbitrary text' do
end
RUBY
end
end
end