Print deprecation message when implicit block expectation syntax is used #1139

pirj · 2019-10-29T14:42:29Z

The purpose of this (rather controversial) change is to discourage the usage of a so-called implicit block expectation syntax:

subject { -> { do_something } }
it { is_expected.to change(something).to(new_value) }

In case this syntax is being used, a deprecation warning will be printed.

This syntax was referenced to as:

pretty obtuse and not something I’d recommend, generally.
#805 (comment)

We should mention they’re not supported.
#805 (comment)

From rspec-core source:

The one-liner syntax only works with non-block expectations (e.g. expect(obj).to eq, etc) and it cannot be used with block expectations (e.g. expect { object }).

This is a less strict follow-up to #1125, which was released as part of a patch release 3.8.5, and later reverted in 3.8.6 due to a number of complaints.
Even though workarounds are possible for the most desperate usages of this syntax, introducing a deprecation warning in pre-4.0 version of RSpec and removal of (coincidental, but still) support for the implicit block syntax in a major release makes more sense than removal of it in a minor/patch version, since it's used quite often across different codebases I've seen.

Example Deprecation Message

Deprecation Warnings:

You must pass a block rather than an argument to `expect` to use the provided block expectation matcher (change `1`), or the matcher must implement `supports_value_expectations?`.

I'm not quite certain it's the best one possible, since it does not refer to the code that led to this deprecation message.
Appreciate any hint how to make it better.

Additional Things to Notice

According to #1125 (comment), the following works with no disruptions:

RSpec.describe 'introspect' do
  it do
    expect(-> {}).to be_a(Proc)
  end
end

Related Links

#526
#530
#536
#1066
#1125

https://blog.rubystyle.guide/rspec/2019/07/17/rspec-implicit-block-syntax.html
https://www.reddit.com/r/ruby/comments/cejl3q/call_for_discussion_rspec_implicit_block
rubocop/rspec-style-guide#76
rubocop/rubocop-rspec#789

Example fix for matcher libraries that are forced to keep the interface to provide the syntax for their users rodjek/rspec-puppet#767

JonRowe

Sorry for the delay @pirj got a bit exhausted by this change then forgot about!

Changelog.md

lib/rspec/expectations/expectation_target.rb

Changelog.md

lib/rspec/expectations/expectation_target.rb

lib/rspec/matchers/built_in/throw_symbol.rb

spec/spec_helper.rb

lib/rspec/matchers/built_in/yield.rb

pirj · 2020-11-14T18:07:16Z

@JonRowe Is it better finishing this before we release 4.0? Or do you think it will be ok to introduce this warning in 4.0.x and remove in 4.1.0?

JonRowe · 2020-11-15T16:05:56Z

Or do you think it will be ok to introduce this warning in 4.0.x and remove in 4.1.0?

No that would be a breaking change, any removal would be in a major version.

pirj · 2020-11-15T18:21:39Z

Sounds good 👍
I intend to wrap this PR up for 3.11 or 3.12, and handle the removal for 4.0.

Changelog.md

spec/rspec/matchers_spec.rb

lib/rspec/matchers/built_in/change.rb

lib/rspec/expectations/expectation_target.rb

lib/rspec/matchers/built_in/yield.rb

Code review notes addressed

pirj · 2020-11-30T17:09:26Z

The one and only diff-lcs example failure on Travis seems unrelated.

pirj · 2020-12-02T16:59:13Z

spec/support/shared_examples/block_matcher.rb

+  end
+
+  it 'prints a deprecation warning when given a value' do
+    expect_warn_deprecation(/You must pass a block/)


Spotted https://github.com/rspec/rspec-expectations/issues/1233

JonRowe · 2020-12-02T21:36:38Z

I'll try to review this at the weekend, its too large to do during this week with my work schedule atm.

JonRowe · 2020-12-12T10:38:49Z

@pirj this is still on my to do list don't worry, it's just lower than rspec-rails 6.1 support (please don't send ping comments)

https://blog.rubystyle.guide/rspec/2019/07/17/rspec-implicit-block-syntax.html https://rspec.rubystyle.guide/#implicit-block-expectations Spec changes are due to: matcher.matches?(invalid_value) doesn't work with block-only matchers, as no block is passed in, and it fails with a message: 1) RSpec::Matchers::BuiltIn::Change behaves like an RSpec block-only matcher uses the `ObjectFormatter` for `failure_message` Failure/Error: expect(message).to include("detailed inspect") expected "expected `@k` to have changed, but was not given a block" to include "detailed inspect" The redundant (due to existing check in ExpectationTarget) `Proc === @event_proc` checks could not be removed safely as well, since @actual_after is not initialized yet when we haven't executed the block: RuntimeError: Warnings were generated: rspec-dev/repos/rspec-expectations/lib/rspec/matchers/built_in/change.rb:407: warning: instance variable @actual_after not initialized Also see: #1139 #1125

lib/rspec/expectations/expectation_target.rb

JonRowe · 2021-02-05T09:26:49Z

Ok! I have finally reviewed this. I have the following concerns:

That this change is going to be unwelcome based on previous feedback.

I think I'm happy to proceed regardless however, as we can always walk it back if we get enough feedback, and most of these changes are good anyway.

That there is no example of the deprecated code in the deprecation.

e.g.

The implicit block expectation syntax is deprecated, you should pass a block rather than an argument to expect to use the provided block expectation matcher (raise Exception), or the matcher must implement supports_value_expectations?.

I would prefer something like the suggestion I've applied.

https://blog.rubystyle.guide/rspec/2019/07/17/rspec-implicit-block-syntax.html https://rspec.rubystyle.guide/#implicit-block-expectations Spec changes are due to: matcher.matches?(invalid_value) doesn't work with block-only matchers, as no block is passed in, and it fails with a message: 1) RSpec::Matchers::BuiltIn::Change behaves like an RSpec block-only matcher uses the `ObjectFormatter` for `failure_message` Failure/Error: expect(message).to include("detailed inspect") expected "expected `@k` to have changed, but was not given a block" to include "detailed inspect" The redundant (due to existing check in ExpectationTarget) `Proc === @event_proc` checks could not be removed safely as well, since @actual_after is not initialized yet when we haven't executed the block: RuntimeError: Warnings were generated: rspec-dev/repos/rspec-expectations/lib/rspec/matchers/built_in/change.rb:407: warning: instance variable @actual_after not initialized Ruby 1.8-specific workarounds: multiple values for a block parameter (0 for 1) If a block expects an argument, it ought to be provided an argument in 1.8

https://blog.rubystyle.guide/rspec/2019/07/17/rspec-implicit-block-syntax.html https://rspec.rubystyle.guide/#implicit-block-expectations Spec changes are due to: matcher.matches?(invalid_value) doesn't work with block-only matchers, as no block is passed in, and it fails with a message: 1) RSpec::Matchers::BuiltIn::Change behaves like an RSpec block-only matcher uses the `ObjectFormatter` for `failure_message` Failure/Error: expect(message).to include("detailed inspect") expected "expected `@k` to have changed, but was not given a block" to include "detailed inspect" The redundant (due to existing check in ExpectationTarget) `Proc === @event_proc` checks could not be removed safely as well, since @actual_after is not initialized yet when we haven't executed the block: RuntimeError: Warnings were generated: rspec-dev/repos/rspec-expectations/lib/rspec/matchers/built_in/change.rb:407: warning: instance variable @actual_after not initialized Also see: #1139 #1125

pirj · 2021-02-07T15:58:40Z

Sounds good. Applied the suggestion here, and made a similar change in #1285

https://blog.rubystyle.guide/rspec/2019/07/17/rspec-implicit-block-syntax.html https://rspec.rubystyle.guide/#implicit-block-expectations Spec changes are due to: matcher.matches?(invalid_value) doesn't work with block-only matchers, as no block is passed in, and it fails with a message: 1) RSpec::Matchers::BuiltIn::Change behaves like an RSpec block-only matcher uses the `ObjectFormatter` for `failure_message` Failure/Error: expect(message).to include("detailed inspect") expected "expected `@k` to have changed, but was not given a block" to include "detailed inspect" The redundant (due to existing check in ExpectationTarget) `Proc === @event_proc` checks could not be removed safely as well, since @actual_after is not initialized yet when we haven't executed the block: RuntimeError: Warnings were generated: rspec-dev/repos/rspec-expectations/lib/rspec/matchers/built_in/change.rb:407: warning: instance variable @actual_after not initialized Also see: #1139 #1125

JonRowe · 2021-02-07T21:57:31Z

Thanks for your patience and hard work on this :)

…with-value-expectation-target Print deprecation message when implicit block expectation syntax is used

https://blog.rubystyle.guide/rspec/2019/07/17/rspec-implicit-block-syntax.html https://rspec.rubystyle.guide/#implicit-block-expectations Spec changes are due to: matcher.matches?(invalid_value) doesn't work with block-only matchers, as no block is passed in, and it fails with a message: 1) RSpec::Matchers::BuiltIn::Change behaves like an RSpec block-only matcher uses the `ObjectFormatter` for `failure_message` Failure/Error: expect(message).to include("detailed inspect") expected "expected `@k` to have changed, but was not given a block" to include "detailed inspect" The redundant (due to existing check in ExpectationTarget) `Proc === @event_proc` checks could not be removed safely as well, since @actual_after is not initialized yet when we haven't executed the block: RuntimeError: Warnings were generated: rspec-dev/repos/rspec-expectations/lib/rspec/matchers/built_in/change.rb:407: warning: instance variable @actual_after not initialized Also see: #1139 #1125

With [this change](rspec/rspec-expectations#1139) in rspec-expectations, a call to `expect(my_object).to have(...)` is raising a `NoMethodError: undefined method `supports_value_expectations?' for <MyObject:0x000055a1b6e065c0>`. This is because ExpectationTarget calls `supports_value_expectation` here: https://github.com/rspec/rspec-expectations/blob/4e7011fe8ac68b74621336b07f894879c8da202d/lib/rspec/expectations/expectation_target.rb#L127 Which ends up being processed by `Have#method_missing` which sets `supports_value_expectations?` to @collection_name. Later in `Have#determine_collection` it tries to call `supports_value_expectations?` on my_object, which causes the NoMethodError. The fix is simple, just implement `Have#supports_value_expectations?`.

By directly instantiating expectation handler, should/should_not was circumventing enforce_value_expectation check. subject(:action) { -> { raise } } it { should raise_error StandardError } would not print "The implicit block expectation syntax is deprecated", while subject(:action) { -> { raise } } it { is_expected.to raise_error StandardError } will. See rspec/rspec-expectations#1139 --- This commit was imported from rspec/rspec-core@d04af61.

…pirj/warn-on-block-matchers-being-used-with-value-expectation-target Print deprecation message when implicit block expectation syntax is used --- This commit was imported from rspec/rspec-expectations@9efd205.

…pirj/warn-on-block-matchers-being-used-with-value-expectation-target Print deprecation message when implicit block expectation syntax is used --- This commit was imported from rspec/rspec-expectations@f656188.

https://blog.rubystyle.guide/rspec/2019/07/17/rspec-implicit-block-syntax.html https://rspec.rubystyle.guide/#implicit-block-expectations Spec changes are due to: matcher.matches?(invalid_value) doesn't work with block-only matchers, as no block is passed in, and it fails with a message: 1) RSpec::Matchers::BuiltIn::Change behaves like an RSpec block-only matcher uses the `ObjectFormatter` for `failure_message` Failure/Error: expect(message).to include("detailed inspect") expected "expected `@k` to have changed, but was not given a block" to include "detailed inspect" The redundant (due to existing check in ExpectationTarget) `Proc === @event_proc` checks could not be removed safely as well, since @actual_after is not initialized yet when we haven't executed the block: RuntimeError: Warnings were generated: rspec-dev/repos/rspec-expectations/lib/rspec/matchers/built_in/change.rb:407: warning: instance variable @actual_after not initialized Also see: rspec/rspec-expectations#1139 rspec/rspec-expectations#1125 --- This commit was imported from rspec/rspec-expectations@aaf93ad.

pirj self-assigned this Oct 29, 2019

pirj requested a review from JonRowe October 29, 2019 14:42

pirj changed the title ~~The purpose of this (rather controversial) change is to discourage the usage of a so-called implicit block expectation syntax:~~ Print deprecation message when implicit block expectation syntax is used Oct 30, 2019

JonRowe previously requested changes Nov 27, 2019

View reviewed changes

pirj marked this pull request as draft April 27, 2020 21:10

JonRowe changed the base branch from master to main August 2, 2020 02:06

pirj mentioned this pull request Oct 5, 2020

RSpec/ExpectChange doesn't work well with nested class rubocop/rubocop-rspec#1037

Closed

pirj commented Nov 29, 2020

View reviewed changes

pirj force-pushed the warn-on-block-matchers-being-used-with-value-expectation-target branch 2 times, most recently from 0a6314b to f95bb8b Compare November 30, 2020 15:03

pirj marked this pull request as ready for review November 30, 2020 15:10

pirj requested review from JonRowe and benoittgt November 30, 2020 15:10

pirj force-pushed the warn-on-block-matchers-being-used-with-value-expectation-target branch 3 times, most recently from 1039699 to 734c46b Compare November 30, 2020 15:25

pirj force-pushed the warn-on-block-matchers-being-used-with-value-expectation-target branch from 5f18fa7 to e9375db Compare December 2, 2020 00:29

pirj commented Dec 2, 2020

View reviewed changes

pirj force-pushed the warn-on-block-matchers-being-used-with-value-expectation-target branch from 33a8a53 to 8c574e0 Compare December 2, 2020 17:20

This comment has been minimized.

Sign in to view

pirj force-pushed the warn-on-block-matchers-being-used-with-value-expectation-target branch from aaa9683 to ae5ff16 Compare December 2, 2020 21:37

rspec deleted a comment from pirj Dec 12, 2020

pirj mentioned this pull request Jan 28, 2021

Restrict block matchers use with value expectations #1285

Merged

pirj force-pushed the warn-on-block-matchers-being-used-with-value-expectation-target branch from 3a0e06f to f6a761b Compare February 2, 2021 22:03

JonRowe reviewed Feb 5, 2021

View reviewed changes

lib/rspec/expectations/expectation_target.rb Outdated Show resolved Hide resolved

pirj force-pushed the warn-on-block-matchers-being-used-with-value-expectation-target branch from 6e91f8b to fe0c4b7 Compare February 7, 2021 15:51

pirj added a commit that referenced this pull request Feb 7, 2021

fixup! Apply the message change similar to #1139

db7c4e4

pirj requested a review from JonRowe February 7, 2021 16:03

JonRowe merged commit 9efd205 into rspec:main Feb 7, 2021

JonRowe added a commit that referenced this pull request Feb 7, 2021

Merge pull request #1139 from pirj/warn-on-block-matchers-being-used-…

f656188

…with-value-expectation-target Print deprecation message when implicit block expectation syntax is used

pirj deleted the warn-on-block-matchers-being-used-with-value-expectation-target branch February 7, 2021 22:36

This was referenced Feb 10, 2021

Remove a couple of remaining deprecations #1288

Merged

Remove leftovers from #1285 #1289

Merged

guigs mentioned this pull request Mar 4, 2021

Fix for NoMethodError: undefined method `supports_value_expectations?' rspec/rspec-collection_matchers#48

Closed

pirj added this to the 4.0 milestone Mar 12, 2021

pirj mentioned this pull request Jul 18, 2021

Add deprecation warnings for features to be removed/changed in RSpec 4 #1301

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Print deprecation message when implicit block expectation syntax is used #1139

Print deprecation message when implicit block expectation syntax is used #1139

pirj commented Oct 29, 2019 •

edited

Loading

JonRowe left a comment

pirj commented Nov 14, 2020

JonRowe commented Nov 15, 2020 •

edited

Loading

pirj commented Nov 15, 2020

pirj commented Nov 30, 2020

pirj Dec 2, 2020

This comment has been minimized.

JonRowe commented Dec 2, 2020

JonRowe commented Dec 12, 2020

JonRowe commented Feb 5, 2021

pirj commented Feb 7, 2021

JonRowe commented Feb 7, 2021

Print deprecation message when implicit block expectation syntax is used #1139

Print deprecation message when implicit block expectation syntax is used #1139

Conversation

pirj commented Oct 29, 2019 • edited Loading

Example Deprecation Message

Additional Things to Notice

Related Links

JonRowe left a comment

Choose a reason for hiding this comment

pirj commented Nov 14, 2020

JonRowe commented Nov 15, 2020 • edited Loading

pirj commented Nov 15, 2020

pirj commented Nov 30, 2020

pirj Dec 2, 2020

Choose a reason for hiding this comment

This comment has been minimized.

JonRowe commented Dec 2, 2020

JonRowe commented Dec 12, 2020

JonRowe commented Feb 5, 2021

pirj commented Feb 7, 2021

JonRowe commented Feb 7, 2021

pirj commented Oct 29, 2019 •

edited

Loading

JonRowe commented Nov 15, 2020 •

edited

Loading