search

LARailsLearners / box-office

2 stars 1 forks source link

Features#2, gets IMDB id from routes and makes GET request to OMDB to retrieve movie title #14

Closed thomasjinlo closed 6 years ago

thomasjinlo commented 8 years ago

Directions

  1. Go get an api key
  2. Set local ENV as OMDB_API_KEY, with your api key
  3. Run bundle install
kangkyu commented 8 years ago

yes.. you can squash the last two commits, because the last one was empty.

thomas-jung commented 8 years ago

The 3rd commit is empty and even 1st commit is not necessary anymore because you refactored in your own branch before it was merged.

kangkyu commented 8 years ago

Personally I think this change https://github.com/LARailsLearners/box-office/pull/10 should have been merged to master first. It satisfies #2 and https://github.com/LARailsLearners/box-office/pull/10 has been merged... can you write why that change has been reverted somewhere on Github, as a record? @thomas-jung

kangkyu commented 8 years ago

can you make another PR with only first commit? @thomasjinlo

thomas-jung commented 8 years ago

@kangkyu I removed commit for #10 because it was also pushed to master directly instead of through PR, so PR should just be reopened again.

thomasjinlo commented 8 years ago

I think this PR is ready to go, what you guys think? Should I merge or wait?

kangkyu commented 8 years ago

"wait" for sure, we need to go through the code

kangkyu commented 8 years ago

Let's merge if you want @thomasjinlo 😄