rahims / SoCo

SoCo (Sonos Controller) is a simple Python class that allows you to programmatically control Sonos speakers.
331 stars 33 forks source link

Fix for get_current_track_info() when dealing with undefined values #37

Closed JohanElmis closed 11 years ago

JohanElmis commented 11 years ago

I was using get_current_track_info() to find the current playlist-index so I could return to the playlist after doing some announcements. Then I had an error like this:

File "/home/sonos/SoCo/soco.py", line 678, in get_current_track_info track['artist'] = metadata.findtext('.//{http://purl.org/dc/elements/1.1/}creator').encode('utf-8')

Turned out that I didn't have Artist/Title/Album defined in the ID3-tag.

Now it doesn't go down - and returns some useful info along with the playlist position. Problem solved.

DPH commented 11 years ago

Tested and works well - good spot. I'm new to python too so can't advise on better way to code it. Cheers David

rahims commented 11 years ago

Thanks, Johan!

KennethNielsen commented 11 years ago

Hi Johan

This is a nice catch. Thanks for fixing it up.

Even though the request is pulled I thought I would comment a bit on the "more elegant" ways part.

1) The track dictionary is already initialized to contain empty strings in line 644, which makes lines 680, 684 and 688 redundant.

2) I think the syntax if variable is a little disadvantages and it use should be minimized. As far as I remember, the metadata.findtext() function returns None if it does not find anything, so we might as well check for that directly with if variable is not None:

3) You do not need to put parentheses around the boolean statement with ifs in python.

4) In the more cosmetic department, we might do the same thing a little more compactly and without the temporary variable like so: elif d != '' and d != 'NOT_IMPLEMENTED':

Track metadata is returned in DIDL-Lite format

    metadata  = XML.fromstring(d.encode('utf-8'))
    track['title']  = metadata.findtext('.//{http://purl.org/dc/elements/1.1/}title')
    track['artist'] = metadata.findtext('.//{http://purl.org/dc/elements/1.1/}creator')
    track['album']  = metadata.findtext('.//{urn:schemas-upnp-org:metadata-1-0/upnp/}album')
    for key in ['title', 'artist', 'album']:
        if track[key] is None:
            track[key] = ''
        else:
            track[key] = track[key].encode('utf-8')

If you'd like to make any of these minor corrections, just make a separate pull request.