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

Emit a warning for around(:context) #2687

Merged
merged 1 commit into from Jan 12, 2020
Merged

Conversation

pirj
Copy link
Member

@pirj pirj commented Jan 11, 2020

fixes #2486

related to #1031

@pirj pirj self-assigned this Jan 11, 2020
@pirj pirj force-pushed the add-around-context-warning branch 3 times, most recently from aa884c3 to 8c6ff23 Compare January 11, 2020 12:43
Copy link
Member

@benoittgt benoittgt left a comment

Choose a reason for hiding this comment

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

LGTM

Changelog.md Outdated Show resolved Hide resolved
@pirj pirj force-pushed the add-around-context-warning branch from 6672089 to 9d54bbb Compare January 12, 2020 20:15
@pirj pirj requested a review from JonRowe January 12, 2020 20:16
@JonRowe JonRowe merged commit 7a264e9 into master Jan 12, 2020
@JonRowe JonRowe deleted the add-around-context-warning branch January 12, 2020 21:08
@JonRowe
Copy link
Member

JonRowe commented Jan 12, 2020

👍

JonRowe added a commit that referenced this pull request Mar 18, 2020
Emit a warning for `around(:context)`
@shepmaster
Copy link
Contributor

One thing confusing about this warning is that it points to something that isn't literally around(:context):

WARNING: `around(:context)` hooks are not supported and behave like `around(:example). Called from ...spec/controllers/clients_controller_spec.rb:31:in `block (2 levels) in <top (required)>'.
% sed -n 31p spec/controllers/clients_controller_spec.rb
    around(:all) do |example|

@JonRowe
Copy link
Member

JonRowe commented May 4, 2020

As :all is an alias for :context, it is literally the same, theres no way for us to know other than making the warning more verbose, e.g:

around(:context) (or an alias such as :all).

@shepmaster
Copy link
Contributor

Sure, I'm not saying that it's incorrect, just confusing.

@pirj
Copy link
Member Author

pirj commented May 4, 2020

We can probably move this warning deeper, to scope_and_options_from where we still have the original scope argument before normalization.

@shepmaster Would you want to tackle that?

@shepmaster
Copy link
Contributor

Would you want to tackle that?

Thank you for the offer, but it seems unlikely I'll have the time for it 😿.

We only had one case of this warning so my specific pain has passed. I mostly thought to post this in case other people have the same pain, or if y'all start getting a lot of reports.

MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020
yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 6, 2021
…-context-warning

Emit a warning for `around(:context)`

---
This commit was imported from rspec/rspec-core@36c5ad5.
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.

around(:context) does not work as expected but does not provide a warning
5 participants