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
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 7 additions & 3 deletions lib/faraday/deprecate.rb
Expand Up @@ -8,13 +8,17 @@ module Faraday
# @see Faraday::Deprecate
module DeprecatedClass
def self.proxy_class(new_klass)
Class.new(new_klass).tap do |k|
class << k
Class.new(new_klass) do
class << self
extend Faraday::Deprecate
# Make this more human readable than #<Class:Faraday::ClientError>
klass_name = superclass.to_s[/^#<Class:(\w*::\w*)>$/, 1]
klass_name = superclass.to_s[/^#<Class:([\w:]+)>$/, 1]
deprecate :new, "#{klass_name}.new", '1.0'
deprecate :inherited, klass_name, '1.0'

def ===(other)
superclass === other || super
end
end
end
end
Expand Down
89 changes: 89 additions & 0 deletions spec/faraday/deprecate_spec.rb
@@ -0,0 +1,89 @@
# frozen_string_literal: true

RSpec.describe Faraday::DeprecatedClass do
class SampleClass < StandardError
attr_accessor :foo

def initialize(foo = nil)
@foo = foo || :foo
end
end

SampleDeprecatedClass = Faraday::DeprecatedClass.proxy_class(SampleClass)

it 'does not raise error for deprecated classes but prints an error message' do
error_message, foobar = with_warn_squelching { SampleDeprecatedClass.new(:foo_bar) }
expect(foobar).to be_a(SampleClass)
expect(foobar.foo).to eq(:foo_bar)
expect(error_message).to match(
Regexp.new(
'NOTE: SampleDeprecatedClass.new is deprecated; '\
'use SampleClass.new instead. It will be removed in or after version 1.0'
)
)
end

it 'does not raise an error for inherited error-namespaced classes but prints an error message' do
error_message, = with_warn_squelching { Class.new(SampleDeprecatedClass) }

expect(error_message).to match(
Regexp.new(
'NOTE: Inheriting SampleDeprecatedClass is deprecated; '\
'use SampleClass instead. It will be removed in or after version 1.0'
)
)
end

it 'allows backward-compatible class to be subclassed' do
expect {
with_warn_squelching { Class.new(SampleDeprecatedClass) }
}.not_to raise_error
end

it 'allows rescuing of a current error with a deprecated error' do
begin
raise SampleClass, nil
rescue SampleDeprecatedClass
rescue StandardError => exc
fail "rescued #{exc.class.name} instead"
end
end

it 'allows rescuing of a current error with a current error' do
begin
raise SampleClass, nil
rescue SampleClass
rescue StandardError => exc
fail "rescued #{exc.class.name} instead"
end
end

it 'allows rescuing of a deprecated error with a deprecated error' do
begin
raise SampleDeprecatedClass, nil
rescue SampleDeprecatedClass
rescue StandardError => exc
fail "rescued #{exc.class.name} instead"
end
end

it 'allows rescuing of a deprecated error with a current error' do
begin
raise SampleDeprecatedClass, nil
rescue SampleClass
rescue StandardError => exc
fail "rescued #{exc.class.name} instead"
end
end


def with_warn_squelching
stderr_catcher = StringIO.new
original_stderr = $stderr
$stderr = stderr_catcher
result = yield if block_given?
[stderr_catcher.tap(&:rewind).string, result]
ensure
$stderr = original_stderr
end
end
42 changes: 40 additions & 2 deletions spec/faraday/error_spec.rb
Expand Up @@ -56,7 +56,7 @@
end

it 'does not raise an error for inherited error-namespaced classes but prints an error message' do
error_message, = with_warn_squelching { class E < Faraday::Error::ClientError; end }
error_message, = with_warn_squelching { Class.new(Faraday::Error::ClientError) }

expect(error_message).to match(
Regexp.new(
Expand All @@ -67,7 +67,45 @@
end

it 'allows backward-compatible class to be subclassed' do
expect { class CustomError < Faraday::Error::ClientError; end }.not_to raise_error
expect {
with_warn_squelching { Class.new(Faraday::Error::ClientError) }
}.not_to raise_error
end

it 'allows rescuing of a current error with a deprecated error' do
begin
raise Faraday::ClientError, nil
rescue Faraday::Error::ClientError
rescue Faraday::Error => exc
fail "rescued #{exc.class.name} instead"
end
end

it 'allows rescuing of a current error with a current error' do
begin
raise Faraday::ClientError, nil
rescue Faraday::ClientError
rescue Faraday::Error => exc
fail "rescued #{exc.class.name} instead"
end
end

it 'allows rescuing of a deprecated error with a deprecated error' do
begin
raise Faraday::Error::ClientError, nil
rescue Faraday::Error::ClientError
rescue Faraday::Error => exc
fail "rescued #{exc.class.name} instead"
end
end

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.

rescue Faraday::Error => exc
fail "rescued #{exc.class.name} instead"
end
end
end

Expand Down