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

Fix ImplicitSubject within third-party aliases #1291

Closed
wants to merge 1 commit into from
Closed

Fix ImplicitSubject within third-party aliases #1291

wants to merge 1 commit into from

Conversation

inkstak
Copy link

@inkstak inkstak commented Jun 1, 2022

Fix #1290


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

Comment on lines +90 to +91
other_cops.dig('RSpec', 'Language', 'Includes', 'Examples')
.push('epic')
Copy link
Member

Choose a reason for hiding this comment

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

Fantastic! 👍
I've spent a while yesterday trying to find or remember how this is stubbed for your reference to no avail.

@@ -98,6 +107,17 @@ def permits(actions)
RUBY
end

it 'allows `is_expected` in single statement with an alias' do
Copy link
Member

Choose a reason for hiding this comment

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

The root problem was that Includes.examples were not considered, so maybe it makes sense to specify this, e.g. "with an alias to Includes.examples".

On a different thought, its_call doesn't seems to be a very fair alias to e.g. it_behaves_like, as the latter creates an example group.
Perhaps it would make sense to change aliasing in saharspec to RSpec/Language/Examples/Regular just like we have it for its.
This would also make this PR redundant, and would also

See palkan/action_policy#138 for more, but there is some specifics to how e.g. success is used there, without a block to justify adding it as an alias to Include.examples.

What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

It seems legit. I guess we need @zverok's opinion or open a PR on saharspec.

Copy link
Contributor

Choose a reason for hiding this comment

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

TBH, it wasn't me who changed it in saharspec, I just trusted the person who did. If there is a better/more right option, I am all open to it. Might be slow to review a PR (I am in Ukraine, if it says anything), but I am working most of the days, so please feel free to submit!

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I'll preparing a PR on saharspec in the coming days :
changing the third-party configuration is fixing ImplicitSubject but may lead other cops to report false-positives and need more investigations.

--- config/rubocop-rspec.yml
+++ config/rubocop-rspec.yml
@@ -1,13 +1,15 @@
 RSpec:
   Language:
-    Includes:
-      Examples:
+    Examples:
+      Regular:
         - its_block
         - its_call
         - its_map
-        - fits_block
-        - fits_call
-        - fits_map
+      Skipped:
         - xits_block
         - xits_call
         - xits_map
+      Focused:
+        - fits_block
+        - fits_call
+        - fits_map
its_call(123)         { is_expected.to send_message(task_class, :perform).once }
its_call("203000592") { is_expected.to send_message(task_class, :perform).once }
RSpec/RepeatedDescription: Don't repeat descriptions within an example group. (https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/RepeatedDescription)
  its_call(123)         { is_expected.to send_message(task_class, :perform).once }
  ^^^^^^^^^^^^^
RSpec/RepeatedExample: Don't repeat examples within an example group. (https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/RepeatedExample)
  its_call(123)         { is_expected.to send_message(task_class, :perform).once }
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
RSpec/RepeatedDescription: Don't repeat descriptions within an example group. (https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/RepeatedDescription)
  its_call("203000592") { is_expected.to send_message(task_class, :perform).once }
  ^^^^^^^^^^^^^^^^^^^^^
RSpec/RepeatedExample: Don't repeat examples within an example group. (https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/RepeatedExample)
  its_call("203000592") { is_expected.to send_message(task_class, :perform).once }
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Copy link
Member

Choose a reason for hiding this comment

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

@zverok 👋 Thanks for the input. I hope you're safe.
@inkstak Changes look reasonable.
@ka8725 👋 Do you think with the changes above the use case with RSpec/EmptyExampleGroup you've brought up in this PR would be equally solved?

Copy link

Choose a reason for hiding this comment

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

@pirj do you ask if the following config is equal to the one that I made in the PR to saharspec?

RSpec:
  Language:
    Examples:
      Regular:
        - its_block
        - its_call
        - its_map
      Skipped:
        - xits_block
        - xits_call
        - xits_map
      Focused:
        - fits_block
        - fits_call
        - fits_map

I think so, I checked this config and it works well for all my scenarios.

@pirj
Copy link
Member

pirj commented Jun 2, 2022

I'm closing this, as this can be fixed on saharspec side. Thanks everyone involved!
@inkstak Please accept my apologies for the initially wrong suggestion that sank some of your time.

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.

RSpec/ImplicitSubject does not use example methods defined by third-party gems
4 participants