Add code to warn about the should syntax. #326

Merged
merged 3 commits into from Sep 22, 2013

Conversation

Projects
None yet
3 participants
@samphippen
Member

samphippen commented Sep 13, 2013

No description provided.

Add code to warn about the should syntax.
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 13, 2013

Coverage Status

Coverage increased (+0.15%) when pulling dc8406b on samphippen:warn-about-the-should-syntax into 461fdfd on rspec:master.

Coverage Status

Coverage increased (+0.15%) when pulling dc8406b on samphippen:warn-about-the-should-syntax into 461fdfd on rspec:master.

lib/rspec/expectations/syntax.rb
+ def self.warn_about_should_unless_configured(method_name)
+ if @warn_about_should
+ RSpec.deprecate(
+ "Using #{method_name} from the old `:should` syntax without explicitly enabling the syntax.",

This comment has been minimized.

@myronmarston

myronmarston Sep 13, 2013

Member

After seeing the rspec-mocks deprecation in action, I improved it:

rspec/rspec-mocks@fd78578

Removing the trailing period is particularly important as the warning print " is deprecated" after it. Using backticks around the method name is nice, too.

@myronmarston

myronmarston Sep 13, 2013

Member

After seeing the rspec-mocks deprecation in action, I improved it:

rspec/rspec-mocks@fd78578

Removing the trailing period is particularly important as the warning print " is deprecated" after it. Using backticks around the method name is nice, too.

lib/rspec/expectations/syntax.rb
@@ -43,6 +43,23 @@ def default_should_host
@default_should_host ||= ::Object.ancestors.last
end
+ @warn_about_should = false

This comment has been minimized.

@myronmarston

myronmarston Sep 13, 2013

Member

Why are you setting this to false here? (I didn't see anything similar in rspec-mocks).

@myronmarston

myronmarston Sep 13, 2013

Member

Why are you setting this to false here? (I didn't see anything similar in rspec-mocks).

+ configure_syntax(:should)
+ RSpec.should_not_receive(:deprecate)
+ 3.should eq(3)
+ end

This comment has been minimized.

@myronmarston

myronmarston Sep 13, 2013

Member

I think there's a problem with this test. You don't have any code that clears the warn_about_should flag when the should syntax is configured. I think the problem is that the warn_about_should flag has already been cleared by a previous test. These tests should reset the syntaxes to the default before each example to ensure that clean slate and prevent leakage.

@myronmarston

myronmarston Sep 13, 2013

Member

I think there's a problem with this test. You don't have any code that clears the warn_about_should flag when the should syntax is configured. I think the problem is that the warn_about_should flag has already been cleared by a previous test. These tests should reset the syntaxes to the default before each example to ensure that clean slate and prevent leakage.

+
+ expect(RSpec).to receive(:deprecate).with(*expected_arguments)
+ 3.should eq(3)
+ end

This comment has been minimized.

@myronmarston

myronmarston Sep 13, 2013

Member

As I've commented elsewhere, I'd like a spec for the call site in every deprecation warning we add from here forward, so please add one.

@myronmarston

myronmarston Sep 13, 2013

Member

As I've commented elsewhere, I'd like a spec for the call site in every deprecation warning we add from here forward, so please add one.

This comment has been minimized.

@samphippen

samphippen Sep 21, 2013

Member

Done below here :)

@samphippen

samphippen Sep 21, 2013

Member

Done below here :)

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 21, 2013

Coverage Status

Coverage increased (+0.15%) when pulling 285a5aa on samphippen:warn-about-the-should-syntax into 461fdfd on rspec:master.

Coverage Status

Coverage increased (+0.15%) when pulling 285a5aa on samphippen:warn-about-the-should-syntax into 461fdfd on rspec:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 21, 2013

Coverage Status

Coverage increased (+0.15%) when pulling 285a5aa on samphippen:warn-about-the-should-syntax into 461fdfd on rspec:master.

Coverage Status

Coverage increased (+0.15%) when pulling 285a5aa on samphippen:warn-about-the-should-syntax into 461fdfd on rspec:master.

@samphippen

This comment has been minimized.

Show comment
Hide comment
@samphippen

samphippen Sep 22, 2013

Member

@myronmarston can I get a rereview please?

Member

samphippen commented Sep 22, 2013

@myronmarston can I get a rereview please?

@@ -23,6 +23,12 @@ Bug fixes
(Brandon Turner)
* Fix diffing of hashes with object based keys. (Jon Rowe)
+Deprecations:
+
+ * Using the old :should syntax without explicitly configuring it is disabled.

This comment has been minimized.

@myronmarston

myronmarston Sep 22, 2013

Member

It's not disabled; it's deprecated. There's a big difference.

Also, it's nice to use backticks here for :should.

@myronmarston

myronmarston Sep 22, 2013

Member

It's not disabled; it's deprecated. There's a big difference.

Also, it's nice to use backticks here for :should.

@@ -87,6 +87,11 @@ def backtrace_formatter
end
end
+ def reset_syntaxes_to_default
+ self.syntax = [:should, :expect]
+ RSpec::Expectations::Syntax::warn_about_should!

This comment has been minimized.

@myronmarston

myronmarston Sep 22, 2013

Member

Any reason you chose to use :: rather than . to send the warn_about_should! message? I prefer to use . for message sends everywhere even though ruby allows :: for this case.

@myronmarston

myronmarston Sep 22, 2013

Member

Any reason you chose to use :: rather than . to send the warn_about_should! message? I prefer to use . for message sends everywhere even though ruby allows :: for this case.

+ expect(RSpec.configuration.reporter).to receive(:deprecation) do |options|
+ expect(options[:call_site]).to include([file, line].join(':'))
+ end
+end

This comment has been minimized.

@myronmarston

myronmarston Sep 22, 2013

Member

When you define a top-level method like this it adds it to every object in the system. If that's your intention, it's fine to do that, but generally I think that's a bad idea and there's no reason for this method to be get added everywhere.

Instead, can you put it into a module that gets included, like we did in rspec-core?

@myronmarston

myronmarston Sep 22, 2013

Member

When you define a top-level method like this it adds it to every object in the system. If that's your intention, it's fine to do that, but generally I think that's a bad idea and there's no reason for this method to be get added everywhere.

Instead, can you put it into a module that gets included, like we did in rspec-core?

+ end
+
+ def self.warn_about_should_unless_configured(method_name)
+ if @warn_about_should

This comment has been minimized.

@myronmarston

myronmarston Sep 22, 2013

Member

Your indentation is off here.

@myronmarston

myronmarston Sep 22, 2013

Member

Your indentation is off here.

@@ -73,6 +73,7 @@ def delegated?; true; end
end
after do
+ configure_default_syntax
configure_syntax(@orig_syntax)

This comment has been minimized.

@myronmarston

myronmarston Sep 22, 2013

Member

It seems very odd to configure the default syntax, then configure the original syntax here. Why are you doing that? It seems like it's not needed. Any specs below that need the default configured should configure it themselves rather than relying up on this after hook to set that up after a prior spec.

@myronmarston

myronmarston Sep 22, 2013

Member

It seems very odd to configure the default syntax, then configure the original syntax here. Why are you doing that? It seems like it's not needed. Any specs below that need the default configured should configure it themselves rather than relying up on this after hook to set that up after a prior spec.

+ configure_syntax([:should, :expect])
+ expect(RSpec).not_to receive(:deprecate)
+ 3.should eq(3)
+ end

This comment has been minimized.

@myronmarston

myronmarston Sep 22, 2013

Member

This spec never calls configure_default_syntax. That's a problem, because the deprecation warning is only printed on the first usage of should after the default has been configured. As a result, this spec could pass even w/o the @warn_about_should = false logic if the last spec that ran was something like:

        it 'messes it up' do
          configure_default_syntax
          3.should eq(3)
        end

I think that to ensure there's no false positive, you have to explicitly call configure_default_syntax so that you control when that is called relative to the logic here.

@myronmarston

myronmarston Sep 22, 2013

Member

This spec never calls configure_default_syntax. That's a problem, because the deprecation warning is only printed on the first usage of should after the default has been configured. As a result, this spec could pass even w/o the @warn_about_should = false logic if the last spec that ran was something like:

        it 'messes it up' do
          configure_default_syntax
          3.should eq(3)
        end

I think that to ensure there's no false positive, you have to explicitly call configure_default_syntax so that you control when that is called relative to the logic here.

@myronmarston myronmarston merged commit 285a5aa into rspec:master Sep 22, 2013

1 check passed

default The Travis CI build passed
Details
@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Sep 22, 2013

Member

I added a commit and merged it locally. I couldn't push a commit to this PR because it's based on your fork, @samphippen. In the future, please push a branch to rspec so that we can collaborate on it more easily.

Member

myronmarston commented Sep 22, 2013

I added a commit and merged it locally. I couldn't push a commit to this PR because it's based on your fork, @samphippen. In the future, please push a branch to rspec so that we can collaborate on it more easily.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment