Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

on otp-21+ use uri_string to parse uri #93

Merged
merged 1 commit into from
Feb 2, 2021

Conversation

tsloughter
Copy link
Member

This also fixes shit because currently it seems to always be returning undefined for scheme and other properties.

I'd also like to extend the request to include the rest of the information uri_string:parse can give -- like userinfo.

We should re-evaluate what OTP versions to retain support for.

@tsloughter tsloughter requested a review from a team November 14, 2020 22:15
@coveralls
Copy link

coveralls commented Nov 14, 2020

Coverage Status

Coverage increased (+0.07%) to 75.62% when pulling d920130 on tsloughter:use-uri-string into 581ca7a on elli-lib:main.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 75.135% when pulling b99032d on tsloughter:use-uri-string into 581ca7a on elli-lib:main.

@yurrriq
Copy link
Member

yurrriq commented Nov 17, 2020

lgtm except for https://travis-ci.org/github/elli-lib/elli/jobs/743673725. will plan to check on this tomorrow too

@yurrriq
Copy link
Member

yurrriq commented Nov 21, 2020

Won't it always be the case that {undefined, undefined, undefined} = {Scheme, Host, Port} for {abs_path, FullPath}? 👍 to this PR still, and for adding stuff like userinfo and query to the request.

@yurrriq
Copy link
Member

yurrriq commented Nov 21, 2020

Looks like uri_string:dissect_query/1 is failing on name=knut%3D&foo with {error,missing_value,<<>>}

@yurrriq
Copy link
Member

yurrriq commented Nov 21, 2020

Seems like RFC 3986 doesn't specify, but that uri_string:dissect_query/1 doesn't support keys without values, at least on 21.3.

@yurrriq
Copy link
Member

yurrriq commented Nov 21, 2020

aaand. you already patched it 😄 erlang/otp#1840

@tsloughter
Copy link
Member Author

Hah, I don't remember patching that 👍

Did you notice what is the earliest OTP with that patch? We should probably only use uri_string from that version and above.

@yurrriq
Copy link
Member

yurrriq commented Nov 25, 2020

Did you notice what is the earliest OTP with that patch?

Looks like 22.0

We should probably only use uri_string from that version and above.

👍

@yurrriq yurrriq added the 4.0 label Nov 25, 2020
@yurrriq yurrriq mentioned this pull request Nov 25, 2020
@yurrriq yurrriq mentioned this pull request Feb 2, 2021
src/elli_http.erl Show resolved Hide resolved
@yurrriq yurrriq mentioned this pull request Feb 2, 2021
@yurrriq yurrriq merged commit 2f2fafb into elli-lib:main Feb 2, 2021
@yurrriq yurrriq mentioned this pull request Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants