Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 23 additions & 25 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,21 @@ var https = require('https')
var fs = require('fs')

function getImageData (json) {
var images = json.images || json.album.images
var name = json.images ? json.name : json.album.name

var image = images.reduce((x, y) => {
return x.height * x.width > y.height * y.width ? x : y
})

name = `${name}-${image.width}x${image.height}`
let image = {
name: `${json.title}-${json.thumbnail_width}x${json.thumbnail_height}`,
url: json.thumbnail_url
}

return { name: name, url: image.url }
return image;
Copy link
Owner

@ahstro ahstro Jun 22, 2020

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 😉

}

function downloadImage (imageUrl, imageName) {
var outFile = imageName + '.jpg'
var coverFile = fs.createWriteStream(outFile)
console.log(`Downloading '${outFile}'`)
console.log(`Downloading '${outFile}'...`)
https.get(imageUrl, (res) => {
res.pipe(coverFile)
console.log("Done!");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice adds! 🥰

})
}

Expand All @@ -45,27 +42,29 @@ function getJson (res) {
// This will allow usage with all Spotify URLs I'm aware of - play., open.
// and spotify: - but I recommend the latter because it won't contain any
// weird characters, hopefully.

function getApiUrl (str) {
if (/(?:album|artist|track)s?[:/][A-Za-z0-9]{22}/.test(str)) {
return str.replace(/^.*(artist|album|track)s?[:/]([A-Za-z0-9]{22})$/, (p0, p1, p2) => {
return `https://api.spotify.com/v1/${p1}s/${p2}`
})
return `https://open.spotify.com/oembed?url=` + str;
} else {
return false
console.error(str + " does not look like a valid spotify url, so won't be downloaded.");
return false;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logging! 📚🙌

}
}

function startFetching (url) {
var apiUrl = getApiUrl(url)
function startFetching (urls) {
apiUrls = [];

// Exit with error message if the URL is invalid
if (!apiUrl) {
console.log(`'${url}' is not a valid Spotify URL`)
return
}
urls.forEach((url) => {
if(getApiUrl(url)){
apiUrls.push(getApiUrl(url));
}
})

// Commence callback hell
https.get(apiUrl, getJson)
for(x in apiUrls){
// Commence callback hell
https.get(apiUrls[x], getJson);
}
Copy link
Owner

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? 😊

}

module.exports = (args) => {
Expand All @@ -76,6 +75,5 @@ module.exports = (args) => {
}

var urls = typeof args === 'string' ? [args] : args

urls.forEach(startFetching)
startFetching(urls);
}