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

Duplicate options Hash in .timeout call #577

Merged
merged 1 commit into from
Dec 16, 2019
Merged

Duplicate options Hash in .timeout call #577

merged 1 commit into from
Dec 16, 2019

Conversation

antonvolkoff
Copy link
Contributor

When Hash is passed to HTTP.timeout, it's been mutated. So when you pass
a frozen Hash it raises an exception.

Here is an example of the code:

timeout_settings = { connect: 5, write: 2, read: 10 }.freeze
HTTP.timeout(timeout_settings).get('http://example.com')

In this change, I have added a call to .dup on options Hash to create a
copy and avoid mutation of the original Hash.

@tarcieri
Copy link
Member

Looks like it's failing due to the nonexistence of FrozenError on 2.4

@antonvolkoff
Copy link
Contributor Author

@tarcieri Thank you for a quick answer. There are few ways to fix the spec. I would like to run it by you to choose a proper way to handle this issue.

  1. Check for version:
let(:expected_error_klass) do
  Gem::Version.new(RUBY_VERSION) < Gem::Version.new("2.5.0") ? RuntimeError : FrozenError
end

it '...' do
  expect { client }.not_to raise_error(expected_error_klass)
end
  1. Just check for RuntimeError which will work for both cases
# because
FrozenError.superclass #=> RuntimeError

# I can just
expect { client }.not_to raise_error(RuntimeError)

@tarcieri
Copy link
Member

tarcieri commented Dec 15, 2019

@antonvolkoff how about:

# TODO: remove this when support for Ruby 2.4 is dropped
skip if Gem::Version.new(RUBY_VERSION) < Gem::Version.new("2.5.0")

It's EOL in March 2020 so I'd say it's ok to ignore this particular test

Copy link
Member

@ixti ixti left a comment

Choose a reason for hiding this comment

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

Please remove error class constraint in spec and will be good to go.

spec/lib/http_spec.rb Outdated Show resolved Hide resolved
When `Hash` is passed to HTTP.timeout, it's been mutated. So when you pass
a frozen `Hash` it raises an exception.

Here is an example of the code:
```
timeout_settings = { connect: 5, write: 2, read: 10 }.freeze
HTTP.timeout(timeout_settings).get('http://example.com')
```

In this change, I have added a call to .dup on options Hash to create a
copy and avoid mutation of original Hash.
Copy link
Member

@ixti ixti left a comment

Choose a reason for hiding this comment

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

LGTM

@ixti
Copy link
Member

ixti commented Dec 16, 2019

Travis now being really weird. o_O Gonna force merge and figure out why later.

@ixti ixti merged commit 7ce201e into httprb:master Dec 16, 2019
@tarcieri tarcieri mentioned this pull request May 13, 2021
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