search

snoyberg / http-client

An HTTP client engine, intended as a base layer for more user-friendly packages.
280 stars 194 forks source link

Decent way to use `parseRequest` with checked responses? #304

Open mgsloan opened 7 years ago

mgsloan commented 7 years ago

The docs for parseRequest indicate that its checkResponse won't do anything. I looked around for a function to add the same checkResponse as used by parseUrlThrow, but couldn't find one. I'm thinking it makes sense to:

Thoughts?

mgsloan commented 7 years ago

I suppose when using Network.HTTP.Client, the checkResponse field is available, though the haddocks make it look like a normal function. However, even in that case, the default status check doesn't seem to be available as a function.

Note that I'm mainly running into this in the context of https://www.stackage.org/haddock/lts-9.6/http-conduit-2.2.3.2/Network-HTTP-Simple.html , which should also export functions that support this.

snoyberg commented 7 years ago

I'm +1 on setRequestCheckStatus. I'm very weakly -1 on parseRequestThrow, just because the story around these functions is already sufficiently long and annoying.

On Thu, Sep 28, 2017 at 8:49 AM, Michael Sloan notifications@github.com wrote:

The docs for parseRequest indicate that its checkResponse won't do anything. I looked around for a function to add the same checkResponse as used by parseUrlThrow, but couldn't find one. I'm thinking it makes sense to:

  • Add setRequestCheckStatus (opposite of setRequestIgnoreStatus), which adds the same status check as parseUrlThrow. Use this function in the definition of parseUrlThrow.
  • Add parseRequestThrow, which also uses setRequestCheckStatus.

Thoughts?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/snoyberg/http-client/issues/304, or mute the thread https://github.com/notifications/unsubscribe-auth/AADBBwKKpLKCIgL-HmLzmZSfWJmS0X6Pks5smzNigaJpZM4PmwZX .

mgsloan commented 7 years ago

Cool, sounds good! PR incoming

Perhaps Network.HTTP.Simple should also re-export parseUrl / parseUrlThrow? Currently using that API alone there's no way to use the default status check

snoyberg commented 7 years ago

Hmm.... OK, now I'm not sure. If we're talking about just the Network.HTTP.Simple module, your first recommendation (parseRequestThrow) sounds better to me for consistency. It's the other modules that have the backwards compatibility concerns where I'd be worried about adding the new function name.

On Thu, Sep 28, 2017 at 9:05 AM, Michael Sloan notifications@github.com wrote:

Cool, sounds good! PR incoming

Perhaps Network.HTTP.Simple should also re-export parseUrl / parseUrlThrow? Currently using that API alone there's no way to use the default status check

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/snoyberg/http-client/issues/304#issuecomment-332736657, or mute the thread https://github.com/notifications/unsubscribe-auth/AADBBzX1yR5GpGp8r8afkZ99ZCoavpJ2ks5smzcMgaJpZM4PmwZX .