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

Replace Tmdb.Movies.search/2 for Tmdb.Search to match TMDb API #3

Merged
merged 3 commits into from
Sep 6, 2016
Merged

Conversation

agustif
Copy link
Contributor

@agustif agustif commented Sep 1, 2016

as spoken in issue #2

Let me know if it's alright or needs some touches.

@agustif
Copy link
Contributor Author

agustif commented Sep 1, 2016

A sidenote, Tmdb.Search.lists is not working, but I tried via apiary virtual console and got the same error so I'm guessing once it's fixed on TMDb backend it will work so I left it in the code even if not working right now and send a ticket to TMDb about the API error.

@seanabrahams
Copy link
Owner

Nice work, especially with verifying the API endpoint via their apiary documentation and sending TMDb a ticket about it.

What do you think about creating a separate function so we can DRY things up?

defp search_api(endpoint, query, params \\ %{}) do
  params = Map.merge(params, %{"query" => query})
  get!("search/#{endpoint}?#{URI.encode_query(params)}").body
end

Then we can make the rest of the functions look like:

def movies(query, params \\ %{}) do
  search_api("movies", query, params)
end

I haven't tested this but something like it should work and reduce the repetition of code.

@agustif
Copy link
Contributor Author

agustif commented Sep 3, 2016

Sounds cool, I thought something like this could make sense but since I don't really know what I am doing I try to make it work first, refactor later.

Do you want me to give it a try?

@agustif
Copy link
Contributor Author

agustif commented Sep 3, 2016

PS. we have a reply about the lists endpoint

Reply by Travis Bell on Sep, 01 2016 at 06:07PM

I have temporarily removed list searches but this isn't something that will be down permanently.

Being able to search lists will be restored shortly. Thanks for pointing this out.

@agustif
Copy link
Contributor Author

agustif commented Sep 3, 2016

So after a quick test Im getting this adding the defp search_api and replacing def movies for the code you provided.

I tried using the old method use HTTPoison.Base with the 2 defp methods for modyfing the behavior but still doesnt work.

iex(4)> r Tmdb.Search
warning: redefining module Tmdb.Search (current version defined in memory)
lib/tmdb/search/search.ex:1

warning: default arguments in search_api/3 are never used
lib/tmdb/search/search.ex:43

{:reloaded, Tmdb.Search, [Tmdb.Search]}
iex(5)> Tmdb.Search.movies("Compadres")
** (HTTPoison.Error) %Poison.SyntaxError{message: "Unexpected token: <", token: "<"}
(tmdb) lib/tmdb/search/search.ex:2: Tmdb.Search.request!/5
(tmdb) lib/tmdb/search/search.ex:45: Tmdb.Search.search_api/3

@seanabrahams
Copy link
Owner

Yeah, the issue was that in my code I used movies instead of movie:

search_api("movies", query, params)

should be:

search_api("movie", query, params)

@agustif
Copy link
Contributor Author

agustif commented Sep 5, 2016

Feeling stupid to not be able to figure that one out by myself...

I will be sending a new commit with the modifications!!

@agustif
Copy link
Contributor Author

agustif commented Sep 5, 2016

Excuse the undescriptive message on commit, should be: "DRY Tmdb.Search with defp search_api"

Tried it on iex and working everything fine except for lists but not our issue.

@seanabrahams seanabrahams merged commit f60ca5c into seanabrahams:master Sep 6, 2016
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.

2 participants