Add session req_opts #44
Conversation
|
Closes #39 |
lib/shopify/session.ex
Outdated
| @@ -2,6 +2,7 @@ defmodule Shopify.Session do | |||
| @moduledoc false | |||
|
|
|||
| alias Shopify.Config | |||
| alias Shopify.Session | |||
There was a problem hiding this comment.
# Do you prefer this
alias Shopify.Config
alias Shopify.Session
# over this?
alias Shopify.{Config, Session}Was this an intentional choice? I usually prefer the {} notation, but I am willing to change my habbits 😃
There was a problem hiding this comment.
Fixed 👍 - just missed that
| @@ -82,5 +86,7 @@ defmodule Shopify do | |||
| iex> with %Shopify.Session{} <- Shopify.session, do: :passed | |||
| :passed | |||
| """ | |||
| def session, do: Shopify.Session.new() | |||
| def session do | |||
There was a problem hiding this comment.
Is there a reason you prefer this version over the one line version? Personal preferences?
There was a problem hiding this comment.
More of a personal preference. I generally like the consistency of the multi-line function.
lib/shopify/resources/customer.ex
Outdated
| @@ -91,7 +91,12 @@ defmodule Shopify.Customer do | |||
| @doc false | |||
| def send_invite(session, id, %CustomerInvite{} = custom_invite) do | |||
There was a problem hiding this comment.
Since we are already touching this method in this PR, we should rename the variable to customer_invite
There was a problem hiding this comment.
Done. A few items were touched from the formatter.
test/adapters/http_test.exs
Outdated
| assert {:error, %Error{} = error} = HTTP.get(request) | ||
| assert error.source == :httpoison | ||
| assert error.reason == :timeout | ||
|
|
There was a problem hiding this comment.
I think this test absolutely does the job, but it raises a few questions.
The test is very tightly coupled to the implementation, i wonder if it would make sense to mock httpoison here and just test if it receives the opts.
The test title says it should use the opts, but it is actually testing if the opts are passed to httpoison. Should we change the test title here?
There was a problem hiding this comment.
Yeah - wasnt super keen on it either, but I just wanted to make sure that it actually worked. I updated the test with a mock receiver. Still not a huge fan of it, but it will work.
No description provided.