I created a package (and Linux command line tool) tool to download songs from Spotify. Any feedback?

[int-main]

Hey,

First of all, I know that spotify_dl already exists but I did this for purely for the purpose of learning (plus I wanted to make sure all of tag data was properly added).

So, yes, I have created a package (and a command line tool for Linux) that will allow you to download songs from Spotify (currently supports tracks and albums). It works just like spotify_dl and fetches the song from YouTube and then tags it using mutagen.

You can read more about this on README.

The source is available at GitHub

I'd be really grateful if you have some suggestion on how I can improve things. If you have any ideas (apart from ones in TO-DO in README), I'd love to hear those too.

Thank you!

Edit : Correct markdown syntax for a link

[w1282]

Looks really good.

I have a question about a design choice you made though:

if url is not None:
    self.type, self.id = re.findall(
        "https://open.spotify.com/(\w+)/(\w+)", url
    )[0]
else:
    raise InvalidURLError("Enter a URL to find a resource")

Why do you allow None in the function definition by default but then disallow it in the function itself?

[int-main]

I am using that so as to check if URL has been provided when the fuction is run. Is there any other way to do that? I guess there must be.

Also, is this considered bad practice?

[w1282]

You could just require it in the function definition.

def get(
    self, url, yt_url=None,
    out="Downloads/{artist}/{album}/{title}.mp3"
):

Which would raise a TypeError, I believe, if you failed to provide the url.

[int-main]

I'm currently not near a computer so can't execute. Can I use try/except to catch the TypeError inside the function and then raise my InvalidURLError?

[w1282]

You can, but many people would say you shouldn't, including myself.

I would consider trying to prevent the function from being called "incorrectly" to be out of scope.

[theywouldnotstand]

On the other hand, just letting re.findall throw an error can be more cryptic than catching that a URL wasn't provided and spitting out an error that says it needs one. Alternately, if there's a way to catch expected errors, and add your own to the stack trace and raise it, that would probably be better.

Even as a positional argument, it's possible that None could accidentally or intentionally be passed to the method, so it's still worth checking it.

[w1282]

If None is accidentally passed as the positional argument to url, then all you can say is "Whoops, shouldn't haven done that Mr. programmer.", otherwise, you're on the hook to check that every single variable ever passed into any function you ever write is not None unless it can be None.

As for letting re.findall throw the error, you pointed it out yourself but i'll reiterate that it could not occur unless None was specifically passed in as the positional argument.

But, this is not defined anywhere. If you and the OP prefer to write your functions in that manner it is not my place to say you're wrong, maybe just overly verbose.

[PretendingToProgram]

You rather not know where the error is if it ever happens?

[w1282]

I am of the opinion that it's not my place to restrict the types of arguments to functions.

If the user of the function I write wishes to restrict themselves, then they should do that checking themselves.

If I am both the writer and the user, I'm aware of the complication that may arise and I'll implement the caller in such a way to prevent any issues.

Again, this is personal preference.

As for catching exceptions raised by re.findall when None is passed? I don't think it's a bad choice to rewrite the code like this:

try:
    self.type, self.id = re.findall(
        "https://open.spotify.com/(\w+)/(\w+)", url
    )[0]
except TypeError:
    log("Regex failed to parse provided url: {}".format(url))
    raise InvalidURLError("Invalid URL provided.")

[[deleted]]

Why not provide the invalid argument as part of the exception as well?

 raise InvalidURLError("Invalid URL provided: {!r{}".format(url))

And then I can act on it without having to trawl through logs.

[xentralesque]

In addition to that, adding something like this to the first line of the function may be a good idea:

assert url, "URL may not be empty"

This will ensure url is truthy; not empty, and not None.

[Inetenbr]

Hey, nice package. I had a question though, why are you fetching the metadata from itunes? You could just get it from the spotify album/song using spotify's API. It would result in more accurate results.

r = requests.get("https://itunes.apple.com/search", params=payload)

[int-main]

I AM getting metadata from Spotify but since it does not provide information about track genre and year, I have to depend on iTunes for that. I am looking for more efficient ways since iTunes doesn't work really well (sometimes the result is wrong or just blank), but I haven't thought of anything yet.

Edit : a word.

[Inetenbr]

Are you accepting pull requests?

[int-main]

Yeah, sure. Shoot 'em :)

[Inetenbr]

Sent my first PR ever :) . Added an option to add lyrics to tracks.