-
-
Notifications
You must be signed in to change notification settings - Fork 272
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
[Fix #1337] Update the cop to raise error for insufficient descriptions. #1350
[Fix #1337] Update the cop to raise error for insufficient descriptions. #1350
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thank you.
Thanks @ydah! How do I get this merged? Do I need to wait for the other reviewers to approve? |
0cbe7ad
to
50d871d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect the following to pass, but it fails, why?
expect_no_offenses(<<-'RUBY')
it "works" \
"totally fine" do
end
RUBY
Looks like I didn't handle that edge case! updated the check to check for the entire description instead, and added some more test cases! Thanks for the catch! |
38aafe8
to
df850fd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked against https://github.com/pirj/real-world-rspec, no errors, and the cop "just works" 😄
...
/Users/pirj/source/real-world-rspec/sharetribe/spec/models/person_spec.rb:338:9: C: [Correctable] RSpec/ExampleWording: Your test should be more descriptive.
it 'works' do
^^^^^
/Users/pirj/source/real-world-rspec/solidus/core/spec/lib/spree/core/testing_support/factories/address_factory_spec.rb:41:11: C: [Correctable] RSpec/ExampleWording: Your test should be more descriptive.
it 'works' do
^^^^^
57497 files inspected, 273 offenses detected, 273 offenses autocorrectable
Looks great, thank you for the contribution!
DisallowedExamples: | ||
- works |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thinking out loud.
RuboCop is known to override the default if an array configured in the user's configuration. E.g.
RSpec/ContextWording:
Prefixes:
- who
Would remove the default when
/with
/without
prefixes.
It may change in RuboCop 2.0. Still, we provide the future behaviour by using inherit_mode:
/merge
now.
The benefit is that users:
- won't have to copy over the defaults from the default config
- won't have to follow and replicate changes when we change the default config
- use third-party configs that extend our defaults
One downside I can think of is inconsistency of this configuration with the rest of array options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pirj Is there something I would need to do to address this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it is fine. Just thinking out loud to get an opinion of maintainers.
For me, this change is totally fine as is.
df850fd
to
f074400
Compare
I have renamed some of the required CI checks, which unfortunately means that you will need to rebase this branch to make CI pass. |
ced82d0
to
13fd06c
Compare
Rebased and green @bquorning @Darhazer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cop will also look for insufficient examples and call them out.
Use the DisallowedExamples setting to prevent unclear or insufficient descriptions.
MSG_INSUFFICIENT_DESCRIPTION = 'Your test should be more descriptive.'
I think we use a more clear terminology. Internally in rspec-core, the first argument to it
is called desc
. So perhaps
This cop will also look for insufficient example descriptions and call them out.
Use the DisallowedExampleDescriptions setting to prevent unclear or insufficient example descriptions.
MSG_INSUFFICIENT_DESCRIPTION = 'Your example description is insufficient.'
b7a022c
to
e90887a
Compare
e90887a
to
ce30e66
Compare
ce30e66
to
31c1dd7
Compare
Thank you for the contribution and your time, @akrox58 ! |
Per issue #1337, I tried to add a generic way to the ExampleWordingCop to call out insufficient descriptions. For now, I've used "works" and "throws errors" as that is a phrase I see used often as well. I didn't use a new cop since insufficient descriptions seemed to be along the same lines as "should not start with should or it" as they are both description related.
Before submitting the PR make sure the following are checked:
master
(if not - rebase it).CHANGELOG.md
if the new code introduces user-observable changes.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 modified an existing cop's configuration options:
VersionChanged
inconfig/default.yml
to the next major version.