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

Gun adapter #304

Merged
merged 12 commits into from
Jul 22, 2019
Merged

Gun adapter #304

merged 12 commits into from
Jul 22, 2019

Conversation

alex-strizhakov
Copy link
Contributor

Adapter for gun - https://github.com/ninenines/gun

Added custom option - max_body.

mix.exs Outdated Show resolved Hide resolved
IO.inspect(msg)
IO.inspect("unsupported message received in read response.")
msg
end

This comment was marked as resolved.

msg ->
IO.inspect(msg)
IO.inspect("unsupported message received in read body.")
msg

This comment was marked as resolved.


msg ->
IO.inspect(msg)
IO.inspect("unsupported message received in read response.")

This comment was marked as resolved.

defp read_response(pid, stream, opts) do
receive do
msg ->
case msg do

This comment was marked as resolved.


receive do
msg ->
case msg do

This comment was marked as resolved.

@codecov
Copy link

codecov bot commented Jun 19, 2019

Codecov Report

Merging #304 into master will decrease coverage by 1.29%.
The diff coverage is 82.69%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #304     +/-   ##
=========================================
- Coverage   95.43%   94.14%   -1.3%     
=========================================
  Files          23       24      +1     
  Lines         460      512     +52     
=========================================
+ Hits          439      482     +43     
- Misses         21       30      +9
Impacted Files Coverage Δ
lib/tesla/adapter/gun.ex 82.69% <82.69%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f5436e...6ad53ea. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 19, 2019

Codecov Report

Merging #304 into master will decrease coverage by 0.58%.
The diff coverage is 93.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #304      +/-   ##
==========================================
- Coverage   95.43%   94.85%   -0.59%     
==========================================
  Files          23       24       +1     
  Lines         460      505      +45     
==========================================
+ Hits          439      479      +40     
- Misses         21       26       +5
Impacted Files Coverage Δ
lib/tesla/adapter/gun.ex 93.61% <93.61%> (ø)
lib/tesla.ex 83.33% <0%> (-4.17%) ⬇️
lib/tesla/middleware/decode_rels.ex 100% <0%> (ø) ⬆️
lib/tesla/builder.ex 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0b5f77...3cb7b37. Read the comment docs.

Copy link
Member

@teamon teamon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NICE! 👏

The only thing I'm not really sure about are the exposed options, I'd rather keep them to the minimum, do not set another custom set of defaults and just refer to gun's documentation instead.

lib/tesla/adapter/gun.ex Outdated Show resolved Hide resolved
lib/tesla/adapter/gun.ex Outdated Show resolved Hide resolved
lib/tesla/adapter/gun.ex Outdated Show resolved Hide resolved
mix.exs Outdated Show resolved Hide resolved
test/tesla/adapter/gun_test.exs Show resolved Hide resolved
@rinpatch
Copy link
Contributor

@teamon could you give this another review please?

@teamon
Copy link
Member

teamon commented Jul 13, 2019

Please rebase to recent master (it includes CI fixes). Once all the tests pass I'd be happy to merge this MR 🚀

@alex-strizhakov
Copy link
Contributor Author

@teamon can elixir 1.4.5 and otp 18 be removed?

@teamon
Copy link
Member

teamon commented Jul 18, 2019 via email

@teamon
Copy link
Member

teamon commented Jul 22, 2019

Dropping support for elixir 1.4 & otp 18 would be a breaking change and would require releasing tesla 2.0.

However, if there is no easy way to provide 1.4 support I'm ok with that, 1.4 caused few other issues already and we have elixir 1.19 already.

@alex-strizhakov
Copy link
Contributor Author

alex-strizhakov commented Jul 22, 2019

How it can be done?
httparrot 1.2.0 requires con_cache 0.13, which can't be compiled on 1.4.5.
cowlib 2.5.0 can be compiled on otp 19+.
I don't see any easy way to fix this.

@teamon
Copy link
Member

teamon commented Jul 22, 2019

@alex-strizhakov And do we have to bump httparrot? :)

@alex-strizhakov
Copy link
Contributor Author

alex-strizhakov commented Jul 22, 2019

@teamon without update of httparrot we need to have {:cowlib, "~> 1.0.2", override: true, optional: true} in mix.exs.

@teamon
Copy link
Member

teamon commented Jul 22, 2019

... and gun requires cowlib.

Ok, let's drop 1.4 & 18 🔥

@alex-strizhakov
Copy link
Contributor Author

... and gun requires cowlib.
Forgot about it 😅

@codecov
Copy link

codecov bot commented Jul 22, 2019

Codecov Report

Merging #304 into master will decrease coverage by 0.58%.
The diff coverage is 93.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #304      +/-   ##
==========================================
- Coverage   95.43%   94.85%   -0.59%     
==========================================
  Files          23       24       +1     
  Lines         460      505      +45     
==========================================
+ Hits          439      479      +40     
- Misses         21       26       +5
Impacted Files Coverage Δ
lib/tesla/adapter/gun.ex 93.61% <93.61%> (ø)
lib/tesla.ex 83.33% <0%> (-4.17%) ⬇️
lib/tesla/middleware/decode_rels.ex 100% <0%> (ø) ⬆️
lib/tesla/builder.ex 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0b5f77...3cb7b37. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 22, 2019

Codecov Report

Merging #304 into master will decrease coverage by 0.24%.
The diff coverage is 93.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #304      +/-   ##
==========================================
- Coverage   95.43%   95.19%   -0.25%     
==========================================
  Files          23       25       +2     
  Lines         460      520      +60     
==========================================
+ Hits          439      495      +56     
- Misses         21       25       +4
Impacted Files Coverage Δ
lib/tesla/adapter/gun.ex 93.61% <93.61%> (ø)
lib/tesla/middleware/retry.ex 93.75% <0%> (-6.25%) ⬇️
lib/tesla/middleware/decode_rels.ex 100% <0%> (ø) ⬆️
lib/tesla/builder.ex 100% <0%> (ø) ⬆️
lib/tesla/middleware/path_params.ex 100% <0%> (ø)
lib/tesla/adapter/hackney.ex 96.66% <0%> (+0.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0b5f77...d3554f1. Read the comment docs.

@teamon teamon merged commit 9795075 into elixir-tesla:master Jul 22, 2019
@alex-strizhakov alex-strizhakov deleted the gun-adapter branch July 22, 2019 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants