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 Strict configuration option to RSpec/PendingWithoutReason #1603

Closed
wants to merge 2 commits into from

Conversation

ydah
Copy link
Member

@ydah ydah commented Mar 16, 2023

This PR is add Strict configuration option to RSpec/PendingWithoutReason.
The following PR was found to be an incorrect fix, so I have reverted and added support for this option.

This option works as before by default, but setting it to Strict: false will make it a violation only in the following cases

  • when a pending or skip method is called with no arguments
  • When a pending/skip is done as metadata for example or example group, but there is no reason for it

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 modified an existing cop's configuration options:

  • Set VersionChanged: "<<next>>" in config/default.yml.

@bquorning
Copy link
Collaborator

I wonder is Strict should be "true" or "false" by default. I would say "false", but that may just be my personal preference.

@ydah
Copy link
Member Author

ydah commented Mar 22, 2023

It is very difficult to decide which is the default.
I would somewhat prefer to leave Strict set to true, but I guess that is also my personal preference.

@bquorning bquorning requested a review from pirj March 22, 2023 09:50
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

We should just ignore this

RSpec.describe 'something' do
  pending 'does something'
end

as it's a way to remind self to write a spec for something, or to indicate that something is yet to be implemented along with a proper spec. Previously discussed here.

Is there a good reason to be strict about this?

spec/rubocop/cop/rspec/pending_without_reason_spec.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/pending_without_reason.rb Outdated Show resolved Hide resolved
@ydah ydah force-pushed the support_option branch 2 times, most recently from 15407c0 to 32ddcab Compare April 6, 2023 16:31
@ydah
Copy link
Member Author

ydah commented Apr 6, 2023

Is there a good reason to be strict about this?

The motivation for this PR was not to make it more strict than before, but rather to make it possible to avoid violations for the following cases depending on the configuration options.

RSpec.describe 'something' do
  pending 'does something'
end

The goal was not to ignore them entirely, but to allow control through configuration options.
As mentioned in #1598 (comment), it is a good reason to be able to consider those without reasons as offenses in a strict sense.

@ydah
Copy link
Member Author

ydah commented Apr 6, 2023

However, it may be valid to simply ignore offenses even without using a configuration option 🤔. In that case, this PR could be closed and we could proceed with the release, unless I have missed something.

ydah added 2 commits April 7, 2023 01:44
…nding/skip has a reason inside an example group"

This reverts commit 223da38.
This PR is add `Strict` configuration option to `RSpec/PendingWithoutReason`.
The following PR was found to be an incorrect fix, so I have reverted and added support for this option.
- rubocop#1598

This option works as before by default, but setting it to `Strict: false` will make it a violation only in the following cases
- when a `pending` or `skip` method is called with no arguments
- When a pending/skip is done as metadata for example or example group, but there is no reason for it
@pirj
Copy link
Member

pirj commented Apr 10, 2023

I'd go with just ignoring this case.

@ydah ydah closed this Apr 13, 2023
@ydah ydah deleted the support_option branch April 13, 2023 14:30
@ydah
Copy link
Member Author

ydah commented Apr 13, 2023

I'd go with just ignoring this case.

I agree. So let's leave this case on ignore.

RSpec.describe 'something' do
  pending 'does something'
end

So is it correct to say that the following concerns have been addressed and the blocker in the release is gone? Please point out if I have missed something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants