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

Update RSpec/IteratedExpectation cop documentation with an example of a matcher relying on block arguments #1659

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jprince
Copy link

@jprince jprince commented Jun 8, 2023

This tripped me up but I was able to find a solution via #1206. Updating docs accordingly.


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: "<<next>>" in default/config.yml.

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

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

CHANGELOG.md Outdated Show resolved Hide resolved
@jprince
Copy link
Author

jprince commented Jun 8, 2023 via email

@ydah ydah force-pushed the iterated-expectation-doc branch from 13f81c6 to de6435d Compare June 12, 2023 14:24
Copy link
Member

@ydah ydah left a comment

Choose a reason for hiding this comment

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

Looks good!

@ydah ydah requested review from pirj, bquorning and Darhazer June 12, 2023 14:26
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.

Even though it’s a way to appease the cop, I wouldn’t say it’s readable.
I would suggest extracting the block part i to a custom matcher, or a helper method.
Personally, I’m opposed at documenting this as it is now to prevent this style from being widely used.

@ydah
Copy link
Member

ydah commented Jul 20, 2023

@jprince ping :-)

@jprince
Copy link
Author

jprince commented Jul 21, 2023

Apologies, I didn't realize you were waiting for my response. I don't disagree that the solution I documented is a bit difficult to read. However the current state is that the cop fails and the documentation provides no direction for how you can fix it - custom matcher or otherwise.

IMO it would actually be ideal if the cop ignored this scenario (admittedly I'm not sure how challenging it would be, technically). I think most of us would agree that the alternative (what I've documented here) is worse than .each/.have_attributes and I don't think the original issue (#278) had this scenario in mind.


# good
it 'sets inferred fields for users' do
expect([user1, user2, user3]).to all(satisfy do |user|
Copy link
Member

Choose a reason for hiding this comment

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

Would the following work?
‘’’
expect([user1, user2, user3]).to all have_attributes(
‘’’

Copy link
Author

Choose a reason for hiding this comment

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

That would certainly be more readable, but unfortunately not in this scenario (where block arguments are used). The challenge is that the have_attributes call is receiving the user as an argument. It would raise "undefined local variable or method `user'"

@Darhazer
Copy link
Member

IMO it would actually be ideal if the cop ignored this scenario (admittedly I'm not sure how challenging it would be, technically). I think most of us would agree that the alternative (what I've documented here) is worse than .each/.have_attributes and I don't think the original issue (#278) had this scenario in mind.

I agree that this is not a good use case for the all matcher and the original code might be a better fit. If @rubocop/rubocop-rspec agree with that direction, I'd be happy to make the change in the cop.

If we do not change the cop however, I agree it would be good to have the documentation. Even if a custom matcher might be a more elegant solution, we have to point to at least some way to write the spec.

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.

4 participants