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

Accept URI objects for HTTP integration #895

Merged
merged 1 commit into from
Nov 21, 2022

Conversation

stefanvermaas
Copy link
Contributor

@stefanvermaas stefanvermaas commented Nov 21, 2022

The HTTP::Client#request allows URI objects and objects responding to #to_s and/or #to_str as a request uri.

The HTTP.rb integration doesn't take this into account and is causing failures on the client side when passing an URI object to the request method.

I missed this during the original implementation and it might cause issues for people passing an URI object to the HTTP gem. Also see; https://github.com/httprb/http/blob/13ce3c2710bc27b467dda68f908149a2f69643cc/lib/http/request.rb#L82

I'm looking for feedback in regard to the added test cases. I'm not sure if these add any extra value, but I'd love to hear more about your thoughts @tombruijn.

The HTTP::Client#request allows URI objects and objects
responding to #to_s and/or #to_str as a request uri.

The HTTP.rb integration doesn't take this into account
and is causing failures on the client side when passing
an URI object to the request method.
Copy link
Member

@tombruijn tombruijn left a comment

Choose a reason for hiding this comment

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

Looks good, and it's good to have a regression test like you added.

@tombruijn tombruijn added the bug label Nov 21, 2022
@tombruijn tombruijn merged commit 43c2ef1 into appsignal:main Nov 21, 2022
tombruijn added a commit that referenced this pull request Nov 21, 2022
@tombruijn
Copy link
Member

I've published the fix in Ruby gem 3.2.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants