-
Notifications
You must be signed in to change notification settings - Fork 187
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
Convert HTTP module into hex_core adapter #990
Convert HTTP module into hex_core adapter #990
Conversation
lib/hex/http.ex
Outdated
|
||
headers = | ||
headers | ||
|> Enum.map(&normalize_header/1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was required because :hex_core
headers are #{binary() => binary()}
and :httpc.request/5
only accepts charlists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to change it to binary inside Hex and only convert to/from charlists when we call the httpc functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about changing this along the hex_core
migration but I think it's easier/better to do it now
lib/hex/http.ex
Outdated
|
||
headers = | ||
headers | ||
|> Enum.map(&normalize_header/1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to change it to binary inside Hex and only convert to/from charlists when we call the httpc functions.
mix.exs
Outdated
] | ||
end | ||
|
||
defp deps(_) do | ||
[] | ||
[ | ||
{:hex_core, "~> 0.9.0"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't add dependencies other than for testing in Hex. Instead we vendor hex_core using this script https://github.com/hexpm/hex/blob/main/scripts/vendor_hex_core.sh. The hex_core module names will be prefixed with mix_
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, I've removed this and renamed the hex_core
references
1c3d8fe
to
14d3a5e
Compare
Can you rebase against main so we can get CI to run? And let us know when it's ready for review again. |
14d3a5e
to
c794b4b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Can you look into why there are timeouts on older Elixir/OTP versions?
c794b4b
to
0b1cd66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
0b1cd66
to
dc52638
Compare
I'm not entirely sure why the timeout was occurring but I found out that |
Thank you @cgerling! |
Part of #738