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.

Sign up for GitHub

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

Jump to bottom

raise error when invalid hash is passed to timeout #752

Open
stoivo opened this issue May 31, 2023 · 1 comment · May be fixed by #754
Open

raise error when invalid hash is passed to timeout #752

stoivo opened this issue May 31, 2023 · 1 comment · May be fixed by #754
Labels
Improvement Pick Me!

Comments

@stoivo
Copy link

stoivo commented May 31, 2023

Hi, love httprb.

I would like to suggest to add some validation of the arguments to timeout. I can see that there are already some but we managed to pass {timeout: 5} as the argument

This is the related code.

http/lib/http/chainable.rb

Lines 93 to 112 in 462d711

def timeout(options)
klass, options = case options
when Numeric then [HTTP::Timeout::Global, {:global => options}]
when Hash then [HTTP::Timeout::PerOperation, options.dup]
when :null then [HTTP::Timeout::Null, {}]
else raise ArgumentError, "Use `.timeout(global_timeout_in_seconds)` or `.timeout(connect: x, write: y, read: z)`."
end
%i[global read write connect].each do |k|
next unless options.key? k
options["#{k}_timeout".to_sym] = options.delete k
end
branch default_options.merge(
:timeout_class => klass,
:timeout_options => options
)
end

I can open a PR of you would want that?
@tarcieri
Copy link
Member

Yes, open a PR with what you're proposing

@stoivo stoivo linked a pull request Jun 8, 2023 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Improvement Pick Me!
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Stricter timeout options parsing stoivo/http
3 participants
@tarcieri @ixti @stoivo