Features#2, gets IMDB id from routes and makes GET request to OMDB to retrieve movie title #14
Conversation
box-office.rb
Outdated
|
|
||
| get '/' do | ||
| 'Hello world!' | ||
| end | ||
|
|
||
| get '/movies/:title' do |title| | ||
| response = HTTParty.get("http://api.themoviedb.org/3/search/movie?query=#{title}&api_key=#{ENV['TMDB_API_KEY']}") |
There was a problem hiding this comment.
like we talked about it, we can put some direction how to set your local env variable on README
There was a problem hiding this comment.
Wait a minute, why is it a "title" not "imdb id"?
There was a problem hiding this comment.
Oh I thought we were searching by title, let me see if TMDb let's you query by IMDB id
|
yes.. you can squash the last two commits, because the last one was empty. |
|
The 3rd commit is empty and even 1st commit is not necessary anymore because you refactored in your own branch before it was merged. |
|
Personally I think this change #10 should have been merged to master first. It satisfies #2 and #10 has been merged... can you write why that change has been reverted somewhere on Github, as a record? @thomas-jung |
|
can you make another PR with only first commit? @thomasjinlo |
thomasjinlo
force-pushed
the
features#2
branch
from
a5bc21a to
cff322c
Compare
|
I think this PR is ready to go, what you guys think? Should I merge or wait? |
|
"wait" for sure, we need to go through the code |
thomasjinlo
changed the title
|
|
||
| get '/' do | ||
| 'Hello world!' | ||
| end | ||
|
|
||
| get '/movies/:imdbid' do |id| | ||
| response = HTTParty.get("http://www.omdbapi.com/?apikey=#{ENV['OMDB_API_KEY']}&i=#{id}&plot=full&r=json") |
There was a problem hiding this comment.
This is probably not good, if we're using dotenv this will most likely change. And also @server-monitor brought up that we should extract all client calls as a separate interface and require it in box-office.
There was a problem hiding this comment.
I'm not sure if we need to add another route just yet. The Hello world route is OK. But if we do TDD, we need to add a test framework first (1 PR), then add the test for the new route and add the route (could be 2 or 1 PRs? I don't know.). If we're not going to do TDD, it could be chaotic but also fun.
There was a problem hiding this comment.
let's keep it for this PR, we can change it later when we need. It doesn't hurt DRY not just yet (because even if you extract code, you would use the code only once)
|
Let's merge if you want @thomasjinlo 😄 |
Directions