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

0.1x deprecate fixes #1059

Merged
merged 6 commits into from Oct 19, 2019
Merged

0.1x deprecate fixes #1059

merged 6 commits into from Oct 19, 2019

Conversation

technoweenie
Copy link
Member

@technoweenie technoweenie commented Oct 17, 2019

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 all Faraday::Error::* references for Faraday v0.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.

-/^#<Class:(\w*::\w*)>$/
+/^#<Class:(\w{1}[\w:]*)>$/

Finally, I copied some of the tests in spec/faraday/error_spec.rb to spec/faraday/deprecate_spec.rb, so that those things are still tested once the Faraday::Error::* stuff is all removed for good. This raised some problems with the CustomError and E classes created in error_spec.rb, so I changed those to use Class.new instead.

@BobbyMcWho: I'd really appreciate your thoughts on this :)

BobbyMcWho
BobbyMcWho previously approved these changes Oct 17, 2019
Copy link
Contributor

@BobbyMcWho BobbyMcWho left a 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

lib/faraday/deprecate.rb Outdated Show resolved Hide resolved
Co-Authored-By: Bobby McDonald <BobbyMcWho@users.noreply.github.com>
Copy link
Member

@olleolleolle olleolleolle left a 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.

it 'allows rescuing of a deprecated error with a current error' do
begin
raise Faraday::Error::ClientError, nil
rescue Faraday::ClientError
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member Author

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)>'

Copy link
Member

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.

Copy link
Member Author

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.

@technoweenie technoweenie merged commit 87003c2 into 0.1x Oct 19, 2019
@technoweenie technoweenie deleted the 0.1x-deprecate-fix branch October 19, 2019 15:53
@olleolleolle
Copy link
Member

The raise_error matchers worked out great!

@BobbyMcWho
Copy link
Contributor

Awesome, thanks for catching those cases I missed originally

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.

None yet

3 participants