-
Notifications
You must be signed in to change notification settings - Fork 13
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
load movies/series details from an external api #55
load movies/series details from an external api #55
Conversation
I am going to look at it in the morning (midnight here), but just as a side note since I merged your last PR, you need to update your fork. Sync Fork (Lanflix Master with your Master)https://help.github.com/articles/syncing-a-fork/ Update branchhttps://stackoverflow.com/questions/3876977/update-git-branches-from-master Otherwise your PR has conflicts (since the |
Yeah.. Sure. I'll update.
Thanks,
Sushma Priyadharssini KV
…On Wed, Oct 3, 2018, 3:21 AM KenavR ***@***.***> wrote:
I am going to look at it in the morning (midnight here), but just as a
side note since I merged your last PR, you need to update your fork.
Sync Fork (Lanflix Master with your Master)
https://help.github.com/articles/syncing-a-fork/
Update branch
https://stackoverflow.com/questions/3876977/update-git-branches-from-master
Otherwise your PR has conflicts (since the /movies/:id route is not part
of your branch).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#55 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AWYabNk4kAoL0LsS17ZcrgNHJR5X4m7Uks5ug9_1gaJpZM4XEuZ1>
.
|
@sushma-priyadharssini Thanks for updating it. The idea here was to load the video files from the filesystem (#19) take the name of the found movie/series/episode, load the corresponding data from tvmaze (#20), puts both together and stores it to the database (#21, #22). To achieve that we need a service or something that has a function taking a name of a show (we may need the same for movies) and returns the data coming back from the API. My suggestion would be to create a new service e.g. var TVMazeService = {
loadDataByName: loadDataByName
}
function loadDataByName(showName, callback) {
request.get("http://api.tvmaze.com/search/shows?q="+ showName, (error, response, body) => {
if(error) {
callback(error);
} else {
callback(undefined, body);
}
});
} Now whenever someone wants to load data from the TVMaze api (like in your REST controller), they can use the service like TVMazeService.loadDataByName("The Office", (error, data){
if(error) { console.dir(error); }
else {
// Do something with the data e.g. returning it in a response
}
}); If you feel comfortable with Promises you can also use those rather than the callbacks from my example above. I am happy to use the PS: I saw that the dependencies need updating (security issues), I will do that now, which means you may need to update your fork/branch again. |
Thanks. I have pushed my changes with the above mentioned suggestions. (Created new service-TVMazeService using node-fetch and also used promise objects wherever required) |
@@ -1,6 +1,7 @@ | |||
const mongoose = require('mongoose'); | |||
const Series = require('../models').Series; | |||
const Movies = require('../models').Movie; | |||
var TVMazeService = require('../services/tvmaze.service'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a const?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this could be a const
, but that's not a deal breaker.
@@ -0,0 +1,13 @@ | |||
const fetch = require('node-fetch'); | |||
|
|||
var TVMazeService = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also out of curiousity, why not use let?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also be a constant, since it doesn't get modified. In the beginning, I tried to keep it ES5 only, but looking at the project currently, it already is a mix between var
and const/let
maybe someone will go through it in the future and update them.
Thanks, looks great, sorry for taking so long reviewing it, it's kinda a busy time for me currently. |
loads movie/series details from the suggested api (https://www.tvmaze.com/api). This needs the npm package 'request'.