Skip to content

[beatport] Exclude invalid musical keys - #3377

Merged
sampsyo merged 5 commits into
beetbox:masterfrom
rhlahuja:beatport-handle-none-key
Sep 28, 2019
Merged

[beatport] Exclude invalid musical keys#3377
sampsyo merged 5 commits into
beetbox:masterfrom
rhlahuja:beatport-handle-none-key

Conversation

@rhlahuja

@rhlahuja rhlahuja commented Sep 28, 2019

Copy link
Copy Markdown
Contributor

Fix a bug introduced in #3372 by defaulting the musical_key to None for Beatport releases, avoiding the following:

Traceback (most recent call last):
  File "/Users/rahulahuja/Dropbox/.virtualenvs/beetbox/beets/bin/beet", line 11, in <module>
    load_entry_point('beets', 'console_scripts', 'beet')()
  File "/Users/rahulahuja/git/beetbox/beets/beets/ui/__init__.py", line 1267, in main
    _raw_main(args)
  File "/Users/rahulahuja/git/beetbox/beets/beets/ui/__init__.py", line 1254, in _raw_main
    subcommand.func(lib, suboptions, subargs)
  File "/Users/rahulahuja/git/beetbox/beets/beets/ui/commands.py", line 956, in import_func
    import_files(lib, paths, query)
  File "/Users/rahulahuja/git/beetbox/beets/beets/ui/commands.py", line 926, in import_files
    session.run()
  File "/Users/rahulahuja/git/beetbox/beets/beets/importer.py", line 329, in run
    pl.run_parallel(QUEUE_SIZE)
  File "/Users/rahulahuja/git/beetbox/beets/beets/util/pipeline.py", line 445, in run_parallel
    six.reraise(exc_info[0], exc_info[1], exc_info[2])
  File "/Users/rahulahuja/rahuja Dropbox/Rahul Ahuja/.virtualenvs/beetbox/beets/lib/python3.7/site-packages/six.py", line 693, in reraise
    raise value
  File "/Users/rahulahuja/git/beetbox/beets/beets/util/pipeline.py", line 312, in run
    out = self.coro.send(msg)
  File "/Users/rahulahuja/git/beetbox/beets/beets/util/pipeline.py", line 194, in coro
    func(*(args + (task,)))
  File "/Users/rahulahuja/git/beetbox/beets/beets/importer.py", line 1353, in lookup_candidates
    task.lookup_candidates()
  File "/Users/rahulahuja/git/beetbox/beets/beets/importer.py", line 641, in lookup_candidates
    autotag.tag_album(self.items, search_ids=self.search_ids)
  File "/Users/rahulahuja/git/beetbox/beets/beets/autotag/match.py", line 460, in tag_album
    va_likely):
  File "/Users/rahulahuja/git/beetbox/beets/beets/plugins.py", line 576, in decorated
    for v in generator(*args, **kwargs):
  File "/Users/rahulahuja/git/beetbox/beets/beets/autotag/hooks.py", line 637, in album_candidates
    for candidate in plugins.candidates(items, artist, album, va_likely):
  File "/Users/rahulahuja/git/beetbox/beets/beets/plugins.py", line 386, in candidates
    for candidate in plugin.candidates(items, artist, album, va_likely):
  File "/Users/rahulahuja/git/beetbox/beets/beetsplug/beatport.py", line 362, in candidates
    return self._get_releases(query)
  File "/Users/rahulahuja/git/beetbox/beets/beetsplug/beatport.py", line 418, in _get_releases
    for x in self.client.search(query)]
  File "/Users/rahulahuja/git/beetbox/beets/beetsplug/beatport.py", line 417, in <listcomp>
    albums = [self._get_album_info(x)
  File "/Users/rahulahuja/git/beetbox/beets/beetsplug/beatport.py", line 138, in search
    release = self.get_release(item['id'])
  File "/Users/rahulahuja/git/beetbox/beets/beetsplug/beatport.py", line 155, in get_release
    release.tracks = self.get_release_tracks(beatport_id)
  File "/Users/rahulahuja/git/beetbox/beets/beetsplug/beatport.py", line 168, in get_release_tracks
    return [BeatportTrack(t) for t in response]
  File "/Users/rahulahuja/git/beetbox/beets/beetsplug/beatport.py", line 168, in <listcomp>
    return [BeatportTrack(t) for t in response]
  File "/Users/rahulahuja/git/beetbox/beets/beetsplug/beatport.py", line 263, in __init__
    self.musical_key = six.text_type(data['key'].get('shortName'))
AttributeError: 'NoneType' object has no attribute 'get'

@sampsyo

sampsyo commented Sep 28, 2019

Copy link
Copy Markdown
Member

Looks good overall! Just to check—it's not a problem that, now that the if 'key' in data: check is missing, we'll be setting the field to None more often than before? (Because the field is None by default anyway.)

Could you please add a quick note to the changelog?

@rhlahuja

Copy link
Copy Markdown
Contributor Author

Yup, that shouldn't be an issue because the default is None anyway, like you said.

It seems to me that the if 'key' in data: was always evaluating to True, otherwise we'd hit an AttributeError here with track.musical_key undefined. It's just that sometimes data['key'] is None, in which case we'll now explicitly set musical_key=None instead of throwing AttributeError: 'NoneType' object has no attribute 'get'.

And changelog updated!

@rhlahuja

Copy link
Copy Markdown
Contributor Author

To be safe, I've also changed

if 'bpm' in data:
    self.bpm = data['bpm']

to

self.bpm = data.get('bpm')

to avoid another potential AttributeError here if the bpm key doesn't exist.

@sampsyo

sampsyo commented Sep 28, 2019

Copy link
Copy Markdown
Member

Fantastic. Thank you again!!

@sampsyo
sampsyo merged commit 56d382f into beetbox:master Sep 28, 2019
@snejus snejus added the beatport beatport plugin label Apr 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

beatport beatport plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants