Highjhacker / Ark-Elixir

Ark API Wrapper in Elixir
MIT License
11 stars 0 forks source link

Typespecs of testing #4

Closed insanedefaults closed 6 years ago

insanedefaults commented 6 years ago

This is a first run at adding extremely basic typespecs to a few modules. Some goals of this PR were as follows:

insanedefaults commented 6 years ago

Tagging @nathanjohnson320 as well.

This doesn't even really need to be merged, I mainly threw this up for discussion.

I have generalized the return for nearly all functions as type Api.response(), defined in Ark_Elixir.Api. I did this because I noticed nearly all of the returns are a simple body parse on the HTTP Response. HTTPotion doesn't have any types defined so I made the response type from a very general map.

# any types?
iex(1)> t HTTPotion
No type information for HTTPotion was found

# well, maybe there is something hiding
iex(6)> t HTTPotion.Response
No type information for HTTPotion.Response was found
iex(7)> s HTTPotion.Response
No specification for HTTPotion.Response was found

# wow I would have bet they had a custom response struct
iex(9)> %HTTPotion.Response{}
%HTTPotion.Response{body: nil, headers: [], status_code: nil}

# so they do but they don't type it... well okay then

In the end, I suspect that Ark_Elixir will see breaking changes in regards to function signatures in every regard so I don't want to go too crazy defining what is there yet.

If this looks like it will work for now, I can typespec the rest of the modules and we can get this merged in. Note that the target is not master, it is the other test branch.

nathanjohnson320 commented 6 years ago

This looks like it will work well 👍

I feel like we should switch over to HTTPoison or hackney since those have well defined typespecs and structs which would make our lives easier when implementing our own specs. But I'm not sure if now is the best time to do that or if we should wait after this refactor is finished. What do you all think?

Highjhacker commented 6 years ago

Indeed it looks great ! I agree to switching over HTTPoison or Hackney (or Tesla ? What do you think about this one ?), didn't know in the first place that HTTPotion wasn't a good choice. I can probably do the rewriting tonight, let me know about what you think is the better choice between HTTPoison, Hackney and Tesla.

insanedefaults commented 6 years ago

@Highjhacker everything should be ready to go now for the first pass at this. In the future, after the api is settled a bit, I will look into adding more specific custom typespecs and building off of things like HTTPoison.Response.t.