search

mrkkrp / req

An HTTP client library
Other
337 stars 40 forks source link

Incorrect parsing for url with fragment #73

Closed geekingfrog closed 4 years ago

geekingfrog commented 4 years ago

The following url, when parsed, will returns a 404: https://github.com/tweag/porcupine#README.md. When showing the url, I get Url Https ("porcupine#README.md" :| ["tweag","github.com"])

This can probably be fixed with a ByteString.takeWhile (/= '#') in https://github.com/mrkkrp/req/blob/master/Network/HTTP/Req.hs#L905

mrkkrp commented 4 years ago

It looks like we do not support fragments in URIs. Delegating URI parsing to a proper URI library like modern-uri looks reasonable. Not sure when I'll get to doing this though.

mrkkrp commented 4 years ago

As a matter of fact I don't think we have a proper support for fragments in req, this also should be fixed. I think I may have time in 4-8th October period to work on req a bit.

geekingfrog commented 4 years ago

would the parsing functions from network-uri work?

mrkkrp commented 4 years ago

Why use network-uri if modern-uri exists?

geekingfrog commented 4 years ago

Oh, wasn't aware of that. I thought it was a lib you wanted to make :/

mrkkrp commented 4 years ago

@geekingfrog I wanted to add support for fragments but it looks like http-client doesn't support them.

mrkkrp commented 4 years ago

Here we go, I opened an issue: https://github.com/snoyberg/http-client/issues/424. If you want to help, feel free to implement this and ask Michael to release a new version of http-client. I'll then include a function fragment :: Text -> Option scheme in req-3.0.0.

mrkkrp commented 4 years ago

I'm going to release version 3.0.0 without this feature because it's easy to add it in backward-compatible way later when http-client starts to support fragments.

louwers commented 4 years ago

@mrkkrp Looks like a WONTFIX for http-client and probably should be a WONTFIX for req as well for the same reasons mentioned.