Prettier/faster code + sensible (?) defaults

[jimpil]

• Makes code more readable/idiomatic
• Removes reflective calls (only a few remain in tests)
• Adds sensible (?) defaults for request.timeout (2 seconds), request.scheme (HTTPS), client.redirect-policy (NORMAL) & client.version (HTTP2)
• Reworks tests that utilised defaults which changed (mainly the redirect-policy related ones)

[jimpil]

Hi there,

First of all, many thanks for your work on this library :+1: . We want to use it at work and so I took on the responsibility of auditing it. Overall, I like what you've done - I just think that the parts of the code can be be made more idiomatic, certain reflective calls don't need to be there, and finally some nice-to-have defaults are missing. This PR addresses this...

Let me know what you think. :)

[gnarroway]

Thanks for your interest. I’ll have a look at your changes and see what works.

Some quick notes on the defaults. In general I will probably not change them since they will break existing consumers and it is also less surprising to align with the underlying client.

• http2 is already the default
• there’s an existing issue regarding timeout defaults which mentions 3minutes. 2 seconds is definitely too aggressive but depending on some kind of consensus I would be open to providing a default in future
• usually (and as documented) you pass in a full URL which gets decomposed into scheme, server-name etc so not sure what value defaulting the scheme provides

[jimpil]

Hi again,

I've just pushed a commit which reverts the defaults I introduced, but keeps/fixes :timeout (apologies for the typo BTW), which is now set to 120000 (2 minutes). I've seen the ticket you're referring to, but to me 3 min is kind of excessive. In any case, something is better than nothing. :+1:

[vincentjames501]

The removing reflective calls will be great to get those errors to stop popping up on our new projects using hato!

[gnarroway]

Thanks @jimpil , I've reviewed all the changes and merged them all in except the timeout (which may go into 1.0.0).

In terms of usage, do you typically create a single HttpClient per app or per data source? I'm just wondering what the impact of the timeout will be.

[vincentjames501]

In terms of usage, do you typically create a single HttpClient per app or per data source? I'm just wondering what the impact of the timeout will be.

FWIW, we tend to do one per service.

[jimpil]

Hi there, I understand what you're asking, but I don't understand why you're asking it. The way I see it, whether you're creating multiple instances is purely a question of configuration. In other words, creating multiple identically-configured client instances is an anti-pattern. The docs clearly state that once created the client can be used to send multiple requests (it is immutable).

If you want to control the concurrency of your async requests, that's exactly what the .executor() config option is, which would be a single line addition to hato (I actually saw a ticket requesting it).

To sum up, there is no timeout-impact as such. Or at least, it doesn't relate to the number of instances, but more to the underlying executor (which is fully configurable). Hope that helps :+1: