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

Tesla Adapter #57

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Tesla Adapter #57

wants to merge 18 commits into from

Conversation

jwarlander
Copy link

Based off of the HTTPoison adapter. Work in progress.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 99.474% when pulling bd051db on jwarlander:tesla-adapter into 6d06d94 on inaka:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 99.474% when pulling a903181 on jwarlander:tesla-adapter into 6d06d94 on inaka:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ec40041 on jwarlander:tesla-adapter into 6d06d94 on inaka:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 99.485% when pulling 3042954 on jwarlander:tesla-adapter into 6d06d94 on inaka:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 10b0c50 on jwarlander:tesla-adapter into 6d06d94 on inaka:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5b68a56 on jwarlander:tesla-adapter into 6d06d94 on inaka:master.

Copy link
Member

@flaviogranero flaviogranero left a comment

Choose a reason for hiding this comment

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

when I saw Tesla some days ago, I immediately thought it could solve a limitation of Dayron: we could allow users to plug middlewares to add custom headers to a request, or handling the response json before it's converted to a struct. And it could be done without the need of writing a custom Adapter. Do you think there is path to allow that with the TeslaAdapter?

end)
end

defp build_hackney_options(opts) do
Copy link
Member

Choose a reason for hiding this comment

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

@jwarlander any reason to go low level and building hackney options instead of trying Tesla Dynamic Middlewares to include custom headers and params?

Copy link
Author

Choose a reason for hiding this comment

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

It should definitely be possible to allow for custom middlewares; perhaps using application config? I could also see it as an option on the repo itself, something like:

defmodule MyApp.RestRepo do
  use Dayron.Repo, otp_app: :myapp, adapter_opts: [middlewares: [
    {Tesla.Middleware.Headers, %{"Authorization" => "token: " <> token }}
  ]
end 

However, what do you think about me raising a separate pull request for that, on top of the initial working adapter for Tesla?

As far as the current implementation goes, one could imagine just pushing headers into the options in the main client code, and having middleware perform the rest of the option translation.. Strictly speaking though, I'm not sure if it would make a big difference - I see middlewares mostly as an excellent way to support optional / new behavior, and translating the Dayron options to something that's suitable for Tesla's underlying Hackney adapter isn't really optional :)

Copy link
Author

Choose a reason for hiding this comment

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

I should note though that the response translation ended up as a middleware module because I could find no good way to hook into the Tesla client itself.. then I ended up working outside of the client anyway, and implementing the tesla_call/2 function that could as easily do that part, so I'll admit it's inconsistent ;)

It would probably be pretty nice to split out option handling and response translation both into separate middlewares, put them in their own source files, and clean up the client module. I'll play around with it and see where it goes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants