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

Don't convert port to string on Excon adapter #190

Merged
merged 1 commit into from
Feb 21, 2018

Conversation

elomar
Copy link
Contributor

@elomar elomar commented Feb 20, 2018

since excon internally checks for an integer (https://github.com/excon/excon/blob/master/lib/excon/utils.rb#L29)

Hey, folks! I'm running into an issue with Savon (thus HTTPI) and Excon. I need to seet "omit_default_port: true" or my request fails. omit_default_port depends on the port being 80 or 443, as integers [https://github.com/excon/excon/blob/master/lib/excon/utils.rb#L29]. Since httpi converts to string, the check is never true and the option is ignored.

Any reason in particular for converting to string? Any idea on how to catch this in the specs? :)

@rogerleite
Copy link
Member

Hi @elomar

I don't know why this port is set using to_s, on May/2013 it was implemented like that. Commit here.

No idea how to catch this on specs, since it's heavily based on mocks. 😐

If this change fix the problem, for me is OK.

Do you need a new release with this fix for now?

@savonrb savonrb deleted a comment from coveralls Feb 21, 2018
@savonrb savonrb deleted a comment from coveralls Feb 21, 2018
@elomar
Copy link
Contributor Author

elomar commented Feb 21, 2018

It does fix it, so OK for me too. And I don't neet a release with this fix, it was easy to monkeypatch it locally. It can just go with the next regular release.

@rogerleite rogerleite merged commit c505eda into savonrb:master Feb 21, 2018
@elomar elomar deleted the patch-1 branch February 21, 2018 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants