Demonstrate Parameters for HTTP::Client #5145

HCLarsen · 2017-10-19T18:14:40Z

Adding parameters to an HTTP request seems like a common use case, so some example code would be very useful in the documentation.

oprypin · 2017-10-19T18:22:07Z

src/http/client.cr

+# params = HTTP::Params.encode({ "q" => "test"}) # => q=test
+# response = HTTP::Client.get "http://www.example.com?" + params
+# response.status_code      # => 200
+# response.body.lines.first # => "<!doctype html>"


No need to repeat this from the previous example if it's not relevant.

I think it'd be a good idea to keep at least one of those output lines to show symmetry with the previous example.

oprypin · 2017-10-19T18:23:47Z

src/http/client.cr

+# ```
+# require "http/client"
+#
+# params = HTTP::Params.encode({ "q" => "test"}) # => q=test


No space in {". Run crystal tool format, that might fix it.

This is also what caused the CI failure.

oprypin · 2017-10-19T18:29:31Z

src/http/client.cr

+# require "http/client"
+#
+# params = HTTP::Params.encode({ "q" => "test"}) # => q=test
+# response = HTTP::Client.get "http://www.example.com?" + params


Maybe worth having a more complex example? 2 parameters, one of which contains something that needs to be escaped? {"author" => "John Doe", "offset" => 20}

Ok, those sound good to me.

Although I had to change the second value to a string instead of an int. "20" works, but 20 throws an error.

RX14 · 2017-10-20T10:21:57Z

I strongly dislike using this method for creating queries with params. The correct way is to set the params using a URI object. We have to parse any String values into a URI anyway, so we should probably have a named param for the String version which sets the params on the URI.

HCLarsen · 2017-10-20T15:38:21Z

I like it. It's what I was advised to do when I asked about this use case in the gitter chat. I like that it keeps everything within the HTTP module. I also really like that you can take a hash, and just turn it into a URL encoded string in just one step. Very user friendly.

RX14 · 2017-10-20T15:52:22Z

Surely just adding a named parameter with the URL params is more friendly than manually appending the URL with ? and using HTTP::Params.encode manually.

oprypin · 2017-10-20T15:56:40Z

Can you finally show the better replacement that you're talking about, in an example?

HCLarsen · 2017-10-20T16:01:31Z

I don't see any params method in the URI class, so I'm not sure what you're getting at. I agree that an example would help us understand what you're suggesting.

RX14 · 2017-10-20T16:03:45Z

HTTP::Client.get("http://www.example.com", query_params: {"foo" => "bar"}) would be ideal but currently you need to do

uri = URI.parse("http://www.example.com")
uri.query = HTTP::Params.encode({ "q" => "test"})
HTTP::Client.get(uri)

HCLarsen · 2017-10-20T16:09:27Z

I see how that works, but I don't think it's inherently better. Now granted, that example should be added to the documentation, but I'd place it in the URI class documentation. Quite frankly, having just looked through that documentation, I did not see anything to suggest this as possible.

HCLarsen · 2017-10-20T18:34:19Z

Also @RX14 , I love the idea of HTTP::Client.get("http://www.example.com", query_params: {"foo" => "bar"}). Would you like me to add an issue for suggesting that?

asterite · 2017-10-20T18:39:07Z

What about:

HTTP::Client.get("http://www.example.com?a=b", query_params: {"foo" => "bar"})

Or even:

HTTP::Client.get("http://www.example.com?foo=baz", query_params: {"foo" => "bar"})

That means HTTP::Client now has to parse the URI, parse the query parameters and combine them with the given ones.

Please, let's not add more and more features for no reason. In fact, can we declare a feature freeze? :-)

HCLarsen · 2017-10-20T19:05:16Z

First, can someone tell me why the checks are failing? Not sure what I did wrong.

HCLarsen · 2017-10-20T19:07:36Z

@asterite, in that case, I would say that the developer is doing a very bad job by writing some params in the address, and others in the params hash. With proper documentation, any good developer would know to put all their params in the params hash, and NOT in the address string. In any case, it seems like a different discussion than this PR, so I'm game for opening an issue if you guys are.

oprypin · 2017-10-20T19:34:50Z

First, can someone tell me why the checks are failing?

crystal tool format that I mentioned earlier

HCLarsen · 2017-10-20T19:51:51Z

Just did that, and it changed files that I haven't touched. Is it supposed to do that?

HCLarsen · 2017-10-20T19:54:42Z

When I ran crystal tool format it changed

src/compiler/crystal/tools/init.cr
src/llvm/lib_llvm_ext.cr

As well as client.cr. Before I commit and push, can I be sure that it won't break anything?

oprypin · 2017-10-20T19:56:12Z

Dang... you might have to run the latest formatter on the latest source code.
make
bin/crystal tool format

HCLarsen · 2017-10-20T19:57:28Z

Make gave me an error.

Could not locate llvm-config

oprypin · 2017-10-20T19:59:17Z

Well, basically you need to do the proper setup for compiling Crystal, in order to contribute to it. But that's not necessary for now. The tool pointed out the problem with the file you changed. Just ignore the rest and move on.

HCLarsen · 2017-10-20T20:00:59Z

Like this?

jkthorne · 2018-05-26T07:36:09Z

@HCLarsen I think the is a good example.

HCLarsen · 2018-05-26T16:23:21Z

This is the first activity on this PR in a few months. Do you guys want to merge this?

jkthorne · 2018-05-29T02:19:02Z

This looks good to me.

jkthorne · 2018-06-12T23:39:21Z

@asterite I hope you dont mind me pinging you but I see you are the last one that posted a doc change. Does this look good to you?

jkthorne · 2018-09-04T20:44:02Z

@HCLarsen what is the status on this?

HCLarsen · 2018-09-04T21:01:01Z

You're asking me? I fixed what I was told to fix, and yet there's been no movement on this in over a year. For all I know, the changes made to the language made since then have now made my example invalid.

jkthorne · 2018-09-05T16:36:39Z

@HCLarsen I dont know why it was not merged I would have liked more docs around this.

@RX14 can we merge this?

HCLarsen · 2018-09-05T18:06:27Z

I would like more docs around anything.

* Demonstrate Parameters for HTTP::Client * Improve HTTP::Client with Params example * Format client.cr

Demonstrate Parameters for HTTP::Client

5fa545d

oprypin requested changes Oct 19, 2017

View reviewed changes

oprypin reviewed Oct 19, 2017

View reviewed changes

HCLarsen added 2 commits October 20, 2017 13:53

Improve HTTP::Client with Params example

7ad4112

Merge Master with Local

489a113

Format client.cr

1121401

bcardiff added the kind:docs label Sep 6, 2018

bcardiff approved these changes Sep 6, 2018

View reviewed changes

RX14 approved these changes Sep 7, 2018

View reviewed changes

RX14 merged commit 625e76a into crystal-lang:master Sep 7, 2018

RX14 added this to the 0.27.0 milestone Sep 7, 2018

RX14 added the kind:feature label Sep 7, 2018

ezrast pushed a commit to ezrast/crystal that referenced this pull request Oct 2, 2018

Demonstrate Parameters for HTTP::Client (crystal-lang#5145)

1c8e63d

* Demonstrate Parameters for HTTP::Client * Improve HTTP::Client with Params example * Format client.cr

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Demonstrate Parameters for HTTP::Client #5145

Demonstrate Parameters for HTTP::Client #5145

HCLarsen commented Oct 19, 2017

oprypin Oct 19, 2017

HCLarsen Oct 20, 2017

oprypin Oct 19, 2017

oprypin Oct 19, 2017

oprypin Oct 19, 2017

HCLarsen Oct 19, 2017

HCLarsen Oct 20, 2017

RX14 commented Oct 20, 2017

HCLarsen commented Oct 20, 2017

RX14 commented Oct 20, 2017

oprypin commented Oct 20, 2017

HCLarsen commented Oct 20, 2017

RX14 commented Oct 20, 2017

HCLarsen commented Oct 20, 2017

HCLarsen commented Oct 20, 2017 •

edited

Loading

asterite commented Oct 20, 2017

HCLarsen commented Oct 20, 2017

HCLarsen commented Oct 20, 2017

oprypin commented Oct 20, 2017

HCLarsen commented Oct 20, 2017

HCLarsen commented Oct 20, 2017

oprypin commented Oct 20, 2017 •

edited

Loading

HCLarsen commented Oct 20, 2017

oprypin commented Oct 20, 2017

HCLarsen commented Oct 20, 2017

jkthorne commented May 26, 2018

HCLarsen commented May 26, 2018

jkthorne commented May 29, 2018

jkthorne commented Jun 12, 2018

jkthorne commented Sep 4, 2018

HCLarsen commented Sep 4, 2018 •

edited

Loading

jkthorne commented Sep 5, 2018

HCLarsen commented Sep 5, 2018

Demonstrate Parameters for HTTP::Client #5145

Demonstrate Parameters for HTTP::Client #5145

Conversation

HCLarsen commented Oct 19, 2017

oprypin Oct 19, 2017

Choose a reason for hiding this comment

HCLarsen Oct 20, 2017

Choose a reason for hiding this comment

oprypin Oct 19, 2017

Choose a reason for hiding this comment

oprypin Oct 19, 2017

Choose a reason for hiding this comment

oprypin Oct 19, 2017

Choose a reason for hiding this comment

HCLarsen Oct 19, 2017

Choose a reason for hiding this comment

HCLarsen Oct 20, 2017

Choose a reason for hiding this comment

RX14 commented Oct 20, 2017

HCLarsen commented Oct 20, 2017

RX14 commented Oct 20, 2017

oprypin commented Oct 20, 2017

HCLarsen commented Oct 20, 2017

RX14 commented Oct 20, 2017

HCLarsen commented Oct 20, 2017

HCLarsen commented Oct 20, 2017 • edited Loading

asterite commented Oct 20, 2017

HCLarsen commented Oct 20, 2017

HCLarsen commented Oct 20, 2017

oprypin commented Oct 20, 2017

HCLarsen commented Oct 20, 2017

HCLarsen commented Oct 20, 2017

oprypin commented Oct 20, 2017 • edited Loading

HCLarsen commented Oct 20, 2017

oprypin commented Oct 20, 2017

HCLarsen commented Oct 20, 2017

jkthorne commented May 26, 2018

HCLarsen commented May 26, 2018

jkthorne commented May 29, 2018

jkthorne commented Jun 12, 2018

jkthorne commented Sep 4, 2018

HCLarsen commented Sep 4, 2018 • edited Loading

jkthorne commented Sep 5, 2018

HCLarsen commented Sep 5, 2018

HCLarsen commented Oct 20, 2017 •

edited

Loading

oprypin commented Oct 20, 2017 •

edited

Loading

HCLarsen commented Sep 4, 2018 •

edited

Loading