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.

Sign up for GitHub

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

Make callback defs types less restrictive #364

Merged
merged 2 commits into from
Oct 16, 2018
Merged

Make callback defs types less restrictive #364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd14138
Make callback type definitions less restrictive
ryanwinchester Oct 15, 2018
3ec42f9
Merge branch 'master' into bugfix/callback-defs-types
ryanwinchester Oct 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions lib/httpoison.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,11 @@ defmodule HTTPoison.Request do
defstruct method: :get, url: nil, headers: [], body: "", params: %{}, options: []

@type method :: :get | :post | :put | :patch | :delete | :options | :head
@type headers :: [{atom, binary}] | [{binary, binary}] | %{binary => binary}
@type url :: binary
@type body :: binary | charlist | iodata | {:form, [{atom, any}]} | {:file, binary}
@type params :: map | keyword | [{binary, binary}]
@type options :: keyword
@type headers :: [{atom, binary}] | [{binary, binary}] | %{binary => binary} | any
@type url :: binary | any
Copy link
Owner

Choose a reason for hiding this comment

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

now that I'm reading this we will accept any for the result of process_request_url right? 🤔

Copy link
Contributor Author

@ryanwinchester ryanwinchester Oct 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Technically we want anything that implements String.Chars protocol (i.e. can work with to_string/1), but we can't really spec for protocols, so... we're kinda forced to give suggestions, and then include any on the end, imo

Same could be said for some of the other properties, too.

For example the :body we can expect {:stream, enumerable} but can't really spec for the Enumerable protocol, so... any :/

Anyways, they can technically pass any where any of the properties have an associated process_x function and they implement it and return the expected type.

We could make it more obvious if we do:

@callback process_request_url(any) :: url

@spec process_request_url(any) :: url
def process_request_url(url), do: process_url(url)

instead of

@callback process_request_url(url) :: url

@spec process_request_url(url) :: url
def process_request_url(url), do: process_url(url)

(even though they'd really be the same effect in the end, it's just visual)

Thoughts?

Copy link
Contributor Author

@ryanwinchester ryanwinchester Oct 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@edgurgel I think it's either this or telling people to cast to string first and close #327 (and possibly tag wontfix)

Copy link
Owner

Choose a reason for hiding this comment

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

Oh I agree with any for the input. I'm wondering about the return being url as any is also acceptable 🤔

Copy link
Contributor Author

@ryanwinchester ryanwinchester Oct 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@edgurgel with this change url includes | any ;)

So really we can put either, but i feel like using url in the return conveys the intention better, even though it doesn't really matter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, I'm fine either way, just let me know which you want and where.

Copy link
Owner

Choose a reason for hiding this comment

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

That's fine then 👍

@type body :: binary | charlist | iodata | {:form, [{atom, any}]} | {:file, binary} | any
@type params :: map | keyword | [{binary, binary}] | any
@type options :: keyword | any

@type t :: %__MODULE__{
method: method,
Expand Down
76 changes: 41 additions & 35 deletions lib/httpoison/base.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,35 +113,37 @@ defmodule HTTPoison.Base do
@callback options!(url, headers) :: Response.t() | AsyncResponse.t()
@callback options!(url, headers, options) :: Response.t() | AsyncResponse.t()

@callback patch(url, term) :: {:ok, Response.t() | AsyncResponse.t()} | {:error, Error.t()}
@callback patch(url, term, headers) ::
@callback patch(url, body) :: {:ok, Response.t() | AsyncResponse.t()} | {:error, Error.t()}
@callback patch(url, body, headers) ::
{:ok, Response.t() | AsyncResponse.t()} | {:error, Error.t()}
@callback patch(url, term, headers, options) ::
@callback patch(url, body, headers, options) ::
{:ok, Response.t() | AsyncResponse.t()} | {:error, Error.t()}

@callback patch!(url, term) :: Response.t() | AsyncResponse.t()
@callback patch!(url, term, headers) :: Response.t() | AsyncResponse.t()
@callback patch!(url, term, headers, options) :: Response.t() | AsyncResponse.t()
@callback patch!(url, body) :: Response.t() | AsyncResponse.t()
@callback patch!(url, body, headers) :: Response.t() | AsyncResponse.t()
@callback patch!(url, body, headers, options) :: Response.t() | AsyncResponse.t()

@callback post(url, term) :: {:ok, Response.t() | AsyncResponse.t()} | {:error, Error.t()}
@callback post(url, term, headers) ::
@callback post(url, body) :: {:ok, Response.t() | AsyncResponse.t()} | {:error, Error.t()}
@callback post(url, body, headers) ::
{:ok, Response.t() | AsyncResponse.t()} | {:error, Error.t()}
@callback post(url, term, headers, options) ::
@callback post(url, body, headers, options) ::
{:ok, Response.t() | AsyncResponse.t()} | {:error, Error.t()}

@callback post!(url, term) :: Response.t() | AsyncResponse.t()
@callback post!(url, term, headers) :: Response.t() | AsyncResponse.t()
@callback post!(url, term, headers, options) :: Response.t() | AsyncResponse.t()
@callback post!(url, body) :: Response.t() | AsyncResponse.t()
@callback post!(url, body, headers) :: Response.t() | AsyncResponse.t()
@callback post!(url, body, headers, options) :: Response.t() | AsyncResponse.t()

# deprecated: Use process_request_headers/1 instead
@callback process_headers(list) :: term
@callback process_response_headers(list) :: term

@callback process_request_body(term) :: body
@callback process_request_body(body) :: body

@callback process_request_headers(headers) :: headers

@callback process_request_options(options) :: options

@callback process_request_url(url) :: url

@callback process_request_params(params) :: params

@callback process_response(response) :: term
Expand All @@ -150,36 +152,40 @@ defmodule HTTPoison.Base do

@callback process_response_chunk(binary) :: term

@callback process_status_code(integer) :: term
@callback process_response_headers(list) :: term

@callback process_response_status_code(integer) :: term

@callback process_url(term) :: url
@callback process_request_url(term) :: url
# deprecated: Use process_response_status_code/1 instead
@callback process_status_code(integer) :: term

# deprecated: Use process_request_url/1 instead
@callback process_url(url) :: url

@callback put(binary) :: {:ok, Response.t() | AsyncResponse.t()} | {:error, Error.t()}
@callback put(binary, term) :: {:ok, Response.t() | AsyncResponse.t()} | {:error, Error.t()}
@callback put(binary, term, headers) ::
@callback put(url) :: {:ok, Response.t() | AsyncResponse.t()} | {:error, Error.t()}
@callback put(url, body) :: {:ok, Response.t() | AsyncResponse.t()} | {:error, Error.t()}
@callback put(url, body, headers) ::
{:ok, Response.t() | AsyncResponse.t()} | {:error, Error.t()}
@callback put(binary, term, headers, options) ::
@callback put(url, body, headers, options) ::
{:ok, Response.t() | AsyncResponse.t()} | {:error, Error.t()}

@callback put!(binary) :: Response.t() | AsyncResponse.t()
@callback put!(binary, term) :: Response.t() | AsyncResponse.t()
@callback put!(binary, term, headers) :: Response.t() | AsyncResponse.t()
@callback put!(binary, term, headers, options) :: Response.t() | AsyncResponse.t()
@callback put!(url) :: Response.t() | AsyncResponse.t()
@callback put!(url, body) :: Response.t() | AsyncResponse.t()
@callback put!(url, body, headers) :: Response.t() | AsyncResponse.t()
@callback put!(url, body, headers, options) :: Response.t() | AsyncResponse.t()

@callback request(atom, binary) :: {:ok, Response.t() | AsyncResponse.t()} | {:error, Error.t()}
@callback request(atom, binary, term) ::
@callback request(atom, url) :: {:ok, Response.t() | AsyncResponse.t()} | {:error, Error.t()}
@callback request(atom, url, body) ::
{:ok, Response.t() | AsyncResponse.t()} | {:error, Error.t()}
@callback request(atom, binary, term, headers) ::
@callback request(atom, url, body, headers) ::
{:ok, Response.t() | AsyncResponse.t()} | {:error, Error.t()}
@callback request(atom, binary, term, headers, options) ::
@callback request(atom, url, body, headers, options) ::
{:ok, Response.t() | AsyncResponse.t()} | {:error, Error.t()}

@callback request!(atom, binary) :: Response.t() | AsyncResponse.t()
@callback request!(atom, binary, term) :: Response.t() | AsyncResponse.t()
@callback request!(atom, binary, term, headers) :: Response.t() | AsyncResponse.t()
@callback request!(atom, binary, term, headers, options) :: Response.t() | AsyncResponse.t()
@callback request!(atom, url) :: Response.t() | AsyncResponse.t()
@callback request!(atom, url, body) :: Response.t() | AsyncResponse.t()
@callback request!(atom, url, body, headers) :: Response.t() | AsyncResponse.t()
@callback request!(atom, url, body, headers, options) :: Response.t() | AsyncResponse.t()

@callback start() :: {:ok, [atom]} | {:error, term}

Expand Down Expand Up @@ -219,7 +225,7 @@ defmodule HTTPoison.Base do
@spec process_request_url(url) :: url
def process_request_url(url), do: process_url(url)

@spec process_request_body(any) :: body
@spec process_request_body(body) :: body
def process_request_body(body), do: body

@spec process_request_headers(headers) :: headers
Expand All @@ -232,7 +238,7 @@ defmodule HTTPoison.Base do
@spec process_request_options(options) :: options
def process_request_options(options), do: options

@spec process_request_params(any) :: params
@spec process_request_params(params) :: params
def process_request_params(params), do: params

@spec process_response(response) :: any
Expand Down