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

Using uri.path instead of String brakes HTTP::Client#get #6729

Closed
sudo-nice opened this issue Sep 15, 2018 · 3 comments
Closed

Using uri.path instead of String brakes HTTP::Client#get #6729

sudo-nice opened this issue Sep 15, 2018 · 3 comments

Comments

@sudo-nice
Copy link

sudo-nice commented Sep 15, 2018

Submitting bugs

This code works as expected:

require "http/client"

uri = URI.parse "https://example.com/help"
client = HTTP::Client.new uri
client.get("/help") { |_| puts "hello" }

But with this change it fails:

 uri = URI.parse "https://example.com/help"
 client = HTTP::Client.new uri
+client.get(uri.path) { |_| puts "hello" }
-client.get("/help") { |_| puts "hello" }

I expect uri.path should work, because it holds the same string as "/help":

uri.path == "/help" # => true

carc.in
long error log


Crystal 0.26.1 [3917852] (2018-08-27)
LLVM: 4.0.0
Default target: x86_64-unknown-linux-gnu

@wooster0
Copy link
Contributor

wooster0 commented Sep 15, 2018

The error happens because uri.path can return nil. (The union type in the error is (String | Nil)).
A way to handle this is:

require "http/client"

uri = URI.parse "https://example.com/help"
client = HTTP::Client.new uri
if path = uri.path
  client.get(path) { |_| puts "hello" }
end

The if path = uri.path check ensures that path is not nil.

@RX14
Copy link
Member

RX14 commented Sep 15, 2018

You probably want uri.path || "/" here, since https://example.com will have a nil path, whereas you probably want that to be the same as https://example.com/

@straight-shoota
Copy link
Member

Note: #6323 would change the return type of URI#path to String and this would have worked as expected from the beginning.

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

No branches or pull requests

4 participants