Regression: httpclient fails to compile without -d:ssl

[nigredo-tori]

After 3ad368f importing httpclient module without -d:ssl results in following error:

lib/pure/httpclient.nim(665, 23) Error: undeclared identifier: 'SslContext'

The problem stems from HttpClientBase definition (previously AsyncHttpClient), which has been made generic. Apparently, generic type definitions do not respect when rules. Here's an example of the same bug:

type A[X] = object
  when false:
    b: NoSuchType

This fails with undeclared identifier: 'NoSuchType'.
As a temporary workaround I tried a following quick hack:

when not defineSsl:
  type SslContext* = void

This seems to work (at least the module compiles), but only if added to net module. If this is added to httpclient (so that the hack remains local), compilation fails with Error: redefinition of 'SslContext', even though SslContext is not defined in this case.

[dom96]

Oh yeah, I forgot to fix this. I didn't realise that a compiler bug was hiding amongst it.

I guess for now we can add that hack to the net module. By the way, please let me know what you think about my changes to the httpclient module, keep in mind that I'm not finished yet.

[dom96]

@Araq Please take a look at this.

[nigredo-tori]

I really like how the whole thing is moving towards deduplicating sync and async code (primary example being multisync). Not sure how far we can get along this road without working try statement in async procs, but still this is wonderful.
One thing that still bugs me though is overreliance on enums (HttpMethod, HttpCode). Sure, there are ways to circumvent these (which lead to duplication in API), but one has to be really careful not to step on a landmine.
As an example, here's the proc that parses a response's HTTP code (from one of your commits):

proc code*(response: Response): HttpCode {.raises: [ValueError].} =
  return parseEnum[HttpCode](response.status)

This looks innocent enough (except restricting the range of acceptable codes), but, if we look at HttpCode closely, we find that it is defined like this:

type HttpCode = enum
  ...
  Http404 = "404 Not Found",
  ...

So now our code won't work if someone switches a status message.
I'd prefer leveraging Nim's dependent type features for this:

type HttpCode = distinct range[100..599]

We get enough type safety this way, with less cognitive load.

[dom96]

I really like how the whole thing is moving towards deduplicating sync and async code

Glad you like it. @zah suggested it way back in the day (before I even implemented async await), and while fixing httpclient I decided I should just implement it instead of duplicating code.

This looks innocent enough (except restricting the range of acceptable codes), but, if we look at HttpCode closely, we find that it is defined like this:
So now our code won't work if someone switches a status message.
I'd prefer leveraging Nim's dependent type features for this:

That would then require me to write ``HttpCode(404)though. I guess I could aliasHttp404` etc. to that. I also think that `HttpCode`'s string values are used somewhere, but I can fix that too.

So yeah, I will do that.