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

Scrape out monkey patching #1245

Merged
merged 2 commits into from
Dec 31, 2020
Merged

Scrape out monkey patching #1245

merged 2 commits into from
Dec 31, 2020

Conversation

pirj
Copy link
Member

@pirj pirj commented Dec 18, 2020

Sibling PRs:

Prerequisite PRs:

http://rspec.info/blog/2013/07/the-plan-for-rspec-3/#zero_monkey_patching_mode

we do want to encourage people to switch to the new syntax, so we plan to make RSpec 3 print a warning on first usage of any the old syntax methods (should, should_not, should_receive, etc) unless the should syntax has been explicitly enabled. This should nudge folks towards the new syntax while keeping RSpec friendly to new users and will pave the way for the old syntax to be disabled by default in RSpec 4.

zero-monkey-patching mode for RSpec... We plan for these config options to become the defaults in RSpec 4.0, so that RSpec 4.0 will have zero monkey patching out of the box.

As for "disabled by default" vs "completely removed" and "default, out of the box" vs "impossible" I can only say that RSpec 4 was probably planned to be released earlier, as:

we'll probably be dropping support for 1.8.7 in RSpec 4

but we've also dropped 1.9, 2.0, 2.1 and 2.2

rspec/rspec-core#2301 (comment)

In RSpec 4, we plan to extract all monkey patching from RSpec and move it into a separate gem, so that monkey patching is opt-in instead of opt-out and users have to explicitly install and load a gem to get it.

rspec-should (or rspec-monkey as it's also about exposing example group DSL in the top-level/Module?) will be released later.

Those using the monkey-patched should syntax are not encouraged to update to RSpec 4 until this gem is extracted.

Those using the globally-exposed DSL are encouraged to use RSpec.describe/RSpec.shared_examples_for instead.

@pirj pirj self-assigned this Dec 18, 2020
@pirj pirj force-pushed the remove-monkey-patching branch 2 times, most recently from d22a6f9 to fbd4401 Compare December 18, 2020 21:39
@pirj pirj marked this pull request as ready for review December 19, 2020 00:52
@pirj pirj force-pushed the remove-monkey-patching branch 2 times, most recently from eb8c832 to 1071be7 Compare December 21, 2020 23:15
README.md Outdated Show resolved Hide resolved
Changelog.md Outdated Show resolved Hide resolved
features/built_in_matchers/README.md Outdated Show resolved Hide resolved
maintenance-branch Outdated Show resolved Hide resolved
@pirj pirj force-pushed the remove-monkey-patching branch 2 times, most recently from 3daf73c to 73c60f5 Compare December 23, 2020 23:26
Changelog.md Outdated Show resolved Hide resolved
@pirj pirj added the rspec 4 label Dec 24, 2020
@pirj pirj force-pushed the remove-monkey-patching branch 2 times, most recently from b330730 to 2926bc9 Compare December 25, 2020 14:02
@pirj
Copy link
Member Author

pirj commented Dec 25, 2020

      NoMethodError:
       undefined method `syntax=' for #<RSpec::Expectations::Configuration:0x000055ba304c1f18>
     # ./spec/rspec/core/configuration_spec.rb:2729:in `block in in_fully_monkey_patched_rspec_environment'

4-0-dev is ATM more like main in terms of monkey-patching. Are you aiming to make 4.0 somehow compatible with it?

I suggest following our practice:

  1. Remove the commit from step 2. We will merge ignoring the failure.
  2. Remove the commit from the other, check it passes with the other commit now on main.
  3. Merge the other.
  4. We will trigger builds for the main 4-0-dev branch of affected repositories to check if everything is in order.

@pirj
Copy link
Member Author

pirj commented Dec 26, 2020

Green 💚

@pirj
Copy link
Member Author

pirj commented Dec 26, 2020

Filed minitest/minitest#861

@JonRowe
Copy link
Member

JonRowe commented Dec 26, 2020

Filed minitest/minitest#861

I've already re-worked our build around it, its just the PR isn't merged yet

@pirj
Copy link
Member Author

pirj commented Dec 26, 2020

Me too minitest/minitest#862 😄

JonRowe added a commit that referenced this pull request Dec 27, 2020
@pirj pirj requested a review from JonRowe December 27, 2020 19:50
@pirj
Copy link
Member Author

pirj commented Dec 29, 2020

Anything left to be done here?

Comment on lines -9 to -20
it "accepts and interacts with a matcher" do
expect(@matcher).to receive(:matches?).with(@target).and_return(true)
expect(@target).to @matcher
end

it "asks for a failure_message when matches? returns false" do
expect(@matcher).to receive(:matches?).with(@target).and_return(false)
expect(@matcher).to receive(:failure_message).and_return("the failure message")
expect {
expect(@target).to @matcher
}.to fail_with("the failure message")
end
Copy link
Member

Choose a reason for hiding this comment

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

Did you find duplicates of these? I thought you were moving them (if so a comment saying "these are duplicates of file:line" etc is helpful when reviewing).

Comment on lines -9 to -12
it "accepts and interacts with a matcher" do
expect(@matcher).to receive(:matches?).with(@target).and_return(true)
expect(@target).to @matcher
end
Copy link
Member Author

Choose a reason for hiding this comment

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

This is covered by more specific specs:

Comment on lines -14 to -20
it "asks for a failure_message when matches? returns false" do
expect(@matcher).to receive(:matches?).with(@target).and_return(false)
expect(@matcher).to receive(:failure_message).and_return("the failure message")
expect {
expect(@target).to @matcher
}.to fail_with("the failure message")
end
Copy link
Member Author

Choose a reason for hiding this comment

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

Covered in:

}.to fail_with("the failure message")
end

context "on interpretters that have BasicObject" do
Copy link
Member Author

Choose a reason for hiding this comment

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

We do not support such interpreters.

Comment on lines -47 to -54
it 'does not break the deprecation check on BasicObject-subclassed proxy objects' do
begin
should_enabled = RSpec::Expectations::Syntax.should_enabled?
RSpec::Expectations::Syntax.enable_should unless should_enabled
proxy_class.new(BasicObject.new).should be_proxied
ensure
RSpec::Expectations::Syntax.disable_should if should_enabled
end
Copy link
Member Author

Choose a reason for hiding this comment

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

We are dropping support for monkey-patching should.

Comment on lines -65 to -70
it "accepts and interacts with a matcher" do
expect(@matcher).to receive(:matches?).with(@target).and_return(false)
allow(@matcher).to receive(:failure_message_when_negated)

expect(@target).not_to @matcher
end
Copy link
Member Author

Choose a reason for hiding this comment

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

Covered in:

Comment on lines -72 to -78
it "asks for a failure_message_when_negated when matches? returns true" do
expect(@matcher).to receive(:matches?).with(@target).and_return(true)
expect(@matcher).to receive(:failure_message_when_negated).and_return("the failure message for should not")
expect {
expect(@target).not_to @matcher
}.to fail_with("the failure message for should not")
end
Copy link
Member Author

Choose a reason for hiding this comment

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

Covered in:

@pirj pirj requested a review from JonRowe December 30, 2020 12:18
@pirj pirj dismissed JonRowe’s stale review December 30, 2020 12:19

References to duplicated examples for a removed spec added

@JonRowe JonRowe force-pushed the remove-monkey-patching branch 3 times, most recently from d983376 to 1ab4671 Compare December 31, 2020 14:20
http://rspec.info/blog/2013/07/the-plan-for-rspec-3/#zero_monkey_patching_mode

> we do want to encourage people to switch to the new syntax, so we plan to make RSpec 3 print a warning on first usage of any the old syntax methods (should, should_not, should_receive, etc) unless the should syntax has been explicitly enabled. This should nudge folks towards the new syntax while keeping RSpec friendly to new users and will pave the way for the old syntax to be disabled by default in RSpec 4.

> zero-monkey-patching mode for RSpec...  We plan for these config options to become the defaults in RSpec 4.0, so that RSpec 4.0 will have zero monkey patching out of the box.

As for "disabled by default" vs "completely removed" and "default, out
of the box" vs "impossible" I can only say that RSpec 4 was probably planned to
be released earlier, as:

> we'll probably be dropping support for 1.8.7 in RSpec 4

but we've also dropped 1.9, 2.0, 2.1 and 2.2

rspec/rspec-core#2301 (comment)

> In RSpec 4, we plan to extract all monkey patching from RSpec and move it into a separate gem, so that monkey patching is opt-in instead of opt-out and users have to explicitly install and load a gem to get it.

`rspec-should` (or `rspec-monkey` as it's also about exposing example
group DSL in the top-level/Module?) will be released later.

Those using the monkey-patched `should` syntax are not encouraged to
update to RSpec 4 until this gem is extracted.

Those using the globally-exposed DSL are encouraged to use
`RSpec.describe`/`RSpec.shared_examples_for` instead.
@JonRowe JonRowe merged commit ebab7e2 into 4-0-dev Dec 31, 2020
@JonRowe JonRowe deleted the remove-monkey-patching branch December 31, 2020 15:33
yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 19, 2021
yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 19, 2021
…rspec/remove-monkey-patching

Scrape out monkey patching

---
This commit was imported from rspec/rspec-expectations@ebab7e2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants