-
Notifications
You must be signed in to change notification settings - Fork 2
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
fixed the access token thing etc #2
base: master
Are you sure you want to change the base?
Conversation
elinahedenius
commented
May 26, 2020
- The old api thing didn't let you get content from it anymore without having an access token (api requires token now #1), so I switched it out for another one
- made the download not stop when one url is invalid
- also wanted to try making a pull request and you're my friend so it's not as scary
…ken and made the thing still download the other images even if one url is incorrect
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.
Looking good, just have a few wishes and questions before being able to merge 😊
|
||
return { name: name, url: image.url } | ||
return image; |
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.
Nice! However, now that we're not doing any magic in the function, would you mind rewriting it as an arrow function? 😇
Edit: Oh, I just remebered, consider reading up on how to write nice commit messages. You'll love it yourself, and other open source projects will be very grateful 😉
https.get(imageUrl, (res) => { | ||
res.pipe(coverFile) | ||
console.log("Done!"); |
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.
Nice adds! 🥰
} else { | ||
return false | ||
console.error(str + " does not look like a valid spotify url, so won't be downloaded."); | ||
return false; |
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.
Logging! 📚🙌
for(x in apiUrls){ | ||
// Commence callback hell | ||
https.get(apiUrls[x], getJson); | ||
} |
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.
Not a huge fan of building arrays imperatively like this. Original idea was that startFetching
would be used for individual urls (feels like it would make it more versatile if wanting to extend the program), although I'd be open to doing it differently. How did your thoughts go around this? 😊