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.

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.

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.

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

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.

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)

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.

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?

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? :-)

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

@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.

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

crystal tool format that I mentioned earlier

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

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?

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

Make gave me an error.

Could not locate llvm-config

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.

Like this?

@HCLarsen I think the is a good example.

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

This looks good to me.

@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?

@HCLarsen what is the status on this?

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.

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

@RX14 can we merge this?

I would like more docs around anything.