search

erunion / showtimes

[deprecated] a movie showtimes API
MIT License
52 stars 14 forks source link

getMovie() Initial Attempt #12

Closed jstart closed 9 years ago

jstart commented 9 years ago

WIP Need to separate theater and showtime parsing out into separate functions to be DRY.

Also quick question: do you have a specific editor you use to format javascript? I don't want to mess up indentation or formatting. Using Brackets right now.

erunion commented 9 years ago

I just use Sublime Text 2. Looks like you changed all the spaces to tabs? Hard to see the actual changes here with the tab changes, mind changing back to 2 space indents?

Since I have plans to abstract the data engine out to support actual APIs, I wonder if it'd be best to pass in something like "Birdman" or "Birdman (2014)" into .getMovie(), instead of using the proprietary Google Movies ID.

jstart commented 9 years ago

Will reformat with 2 space indents asap.

Do you think it would make sense to be able to pass multiple types of IDs and handle each differently? like a hash with imdb_id, fandango_id, google_id, etc. Obviously right now we don't have a way to translate ids, but we could hit different endpoints depending on the type of ID passed in. How do you think we would translate "Birdman" into other movie APIs? I don't think all of them have string query search like Google.

jstart commented 9 years ago

I can totally see why it would make sense to make this way more generic to allow interfacing with other services, but do you think it would make sense to build out these APIs scraping google and then make it generic later?

Let me know if you think this PR is worthwhile or if there is any cleanup or test you would like me to add.

erunion commented 9 years ago

@jstart Sorry, been really busy this week with the day job. Will check out your changes tomorrow or Wednesday.

erunion commented 9 years ago

@jstart Sorry, just now getting around to checking this out.

I think we're okay to not worry right now about multiple services, and can just develop this with Google in mind. This branch looks good, but if you want to write up a quick test case for .getMovie(), then I'll merge it all in.

jstart commented 9 years ago

@jonursenbach Did some final cleanup. I wrote a test previously that fetches movies, then gets the movie details for the first movie id. Didn't want any static movie ids in the test cases as movies go out out of theaters. Do you think I need to cover more cases?

erunion commented 9 years ago

Oh, that's a good point. I think we're good here then. Your cleanup looks good.

erunion commented 9 years ago

Published under 1.2.0. Nice job.