tamland / python-tidal

Python API for TIDAL music streaming service
GNU Lesser General Public License v3.0
410 stars 111 forks source link

Wrong return of Track.get_urls() #298

Open exislow opened 5 hours ago

exislow commented 5 hours ago

So this method https://github.com/tamland/python-tidal/blob/910a7b4b84b313259d3388b19e44818b0c4a414b/tidalapi/media.py#L594-L598

Clearly states, that it returns [str].

But within the else statement we find this code piece return self.urls[0] which only returns a str.

I propose, to match the name get_urls() and the return type [str], that the method should look like this:

    def get_urls(self) -> [str]:
        return self.urls
tehkillerbee commented 4 hours ago

Since we can still return both, I suggest .. -> Union[str, [str]] instead

exislow commented 4 hours ago

I would not vote for this, since the name is suggesting, that URLs (plural) are returned. Thus, everybody would expect a list.

Plus it makes code software easier. Otherwise I need to check every time if a list is returned or not in my own code base, which is super annoying.

tehkillerbee commented 3 hours ago

Still, the typings should reflect the actual type returned.

A better alternative would be to stick to [str] and then return [self.urls[0]]. Then it also corresponds to the method name.

exislow commented 2 hours ago

I am confused. Why do you like to unlist self.urls[0] first just to put it afterwards in a list, like this: [self.urls[0]] and return it. Why you not just return self.urls?

It doesn't matter if its BTS or MPD, self.urls contains always a list of strings.

tehkillerbee commented 2 hours ago

For BTS, only a single URL should be returned as more than one URL does not make sense in this case.

This was the original reason why only the first entry in the list was returned. However, returning multiple types is ugly, hence the reason that I suggested returning only the first URL, but still returning it as a list to fix the typings.

Anyways, just to double check I verified the output of the StreamManifest. With BTS, a list of URLs is returned but only ONE url is returned in this list. So we can just return self.urls directly, as you also suggest.