-
Notifications
You must be signed in to change notification settings - Fork 988
0.1x deprecate fixes #1059
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
0.1x deprecate fixes #1059
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, 1 comment on the regex
Co-Authored-By: Bobby McDonald <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious about the RSpec "not-fail" test cases - the opposite of fail.
Thanks for describing the usage in the description and the tests.
spec/faraday/error_spec.rb
Outdated
it 'allows rescuing of a deprecated error with a current error' do | ||
begin | ||
raise Faraday::Error::ClientError, nil | ||
rescue Faraday::ClientError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we find a way to output a message of success when we pass here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a "not-fail" test case? Is that something you see in the results? What would a message of success here look like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed those examples to use expect { raise ... }.to raise_error(...)
instead. Much shorter, and still tests the correct thing. If I remove the #===
implementation on the inherited meta class that Faraday::DeprecatedClass.proxy_class
creates, I get these failures:
1) Faraday::DeprecatedClass allows rescuing of a current error with a deprecated error
Failure/Error: expect { raise SampleClass, nil }.to raise_error(SampleDeprecatedClass)
expected SampleDeprecatedClass, got #<SampleClass: SampleClass> with backtrace:
# ./spec/faraday/deprecate_spec.rb:44:in `block (3 levels) in <top (required)>'
# ./spec/faraday/deprecate_spec.rb:44:in `block (2 levels) in <top (required)>'
# ./spec/faraday/deprecate_spec.rb:44:in `block (2 levels) in <top (required)>'
2) Faraday::ClientError.initialize maintains backward-compatibility until 1.0 allows rescuing of a current error with a deprecated error
Failure/Error: expect { raise Faraday::ClientError, nil }.to raise_error(Faraday::Error::ClientError)
expected Faraday::Error::ClientError, got #<Faraday::ClientError Faraday::ClientError> with backtrace:
# ./spec/faraday/error_spec.rb:76:in `block (5 levels) in <top (required)>'
# ./spec/faraday/error_spec.rb:76:in `block (4 levels) in <top (required)>'
# ./spec/faraday/error_spec.rb:76:in `block (4 levels) in <top (required)>'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of "if we get here, assert_equal(true, true, "it works")
" kind of thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh I see. I think the expect
syntax works too. I don't think an expect(true).to be_true
underneath is necessary.
The raise_error matchers worked out great! |
Awesome, thanks for catching those cases I missed originally |
This fixes a few issues with #1054
First: You can't rescue a current Faraday exception (
Faraday::ClientError
, for example) with its deprecated error class (Faraday::Error::ClientError
). This would let us remove allFaraday::Error::*
references for Faradayv0.17.1
. We should update them too, so that users don't get a bunch of deprecation warnings they can't fix. Even if that's too risky of a change, I still think this behavior is correct and worth including.Second, the regex for the class name changed so it could grab the class name of a root level class. Only came up because some of the test constants have no namespace.
Finally, I copied some of the tests in
spec/faraday/error_spec.rb
tospec/faraday/deprecate_spec.rb
, so that those things are still tested once theFaraday::Error::*
stuff is all removed for good. This raised some problems with theCustomError
andE
classes created inerror_spec.rb
, so I changed those to useClass.new
instead.@BobbyMcWho: I'd really appreciate your thoughts on this :)