Remediate / refactor wikidata plugin #372
Conversation
1. Reduce log verbosity, particularly INFO but also DEBUG, and provide additional information. 2. Refactor code to reduce nesting, primarily using early returns, but also simplifying and generally cleaning. 3. Fix a few small bugs whose symptoms would probably not be noticed. I have tested this version, however I don't have not yet found any of my music that has Wikidata genres.
|
The MB data I used to test this was release:0cc705d1-ea84-4a28-b6f5-812e295129f0. |
Because we should try to get all genre plugins to create hidden variables named `~genre_` + plugin-name.
|
Ok - this is now working perfectly as far as I can tell. Review may be difficult as it is both a refactor and contains various bug fixes, most notably when wikidata network errors occur, the album will never resolve. Additional functionality: Avoid duplicate albumartist and artist lookups |
|
As an aside, I am unsure how it has a codacy issue of a method being over complex when the refactoring reduced the complexity of the code substantially. However, if needed I can refactor it a little more to reduce the complexity further. |
There was a problem hiding this comment.
Thanks for this detailed refactoring. I'm finally catching up on reviewing plugins. Not much to say really about this, looks good and simplifies the code. Only some small comments.
As an aside, I am unsure how it has a codacy issue of a method being over complex when the refactoring reduced the complexity of the code substantially. However, if needed I can refactor it a little more to reduce the complexity further.
Yes, codacy is just being silly I think. Let's ignore it.
| found = True | ||
| wikidata_url = relation.target[0].text | ||
| self.process_wikidata(Wikidata.ARTIST, wikidata_url, item_id) | ||
| log.info('WIKIDATA: wikidata url found for ARTIST: %s', wikidata_url) |
There was a problem hiding this comment.
This is really minor, but the other code paths have this log message before the self.process_wikidata call. Would be nice to have it consistent.
| found = True | ||
| wikidata_url = relation.target[0].text | ||
| log.info('WIKIDATA: wikidata url found for WORK: %s', wikidata_url) | ||
| self.process_wikidata(Wikidata.WORK, wikidata_url, item_id) |
There was a problem hiding this comment.
What do you think about moving the code to find the wikidata url into a helper function? Something like:
def get_wikidata_url(node):
if 'relation_list' in node.children:
for relation in response.metadata[0].work[0].relation_list[0].relation:
if relation.type == 'wikidata' and 'target' in relation.children:
return relation.target[0].text
return NoneThe use it like:
wikidata_url = response.metadata[0].work[0]
if wikidata_url:
found = True
log.info('WIKIDATA: wikidata url found for WORK: %s', wikidata_url)
self.process_wikidata(Wikidata.WORK, wikidata_url, item_id)| Since genres can be gathered from multiple sources and the genre tag may be overwritten, | ||
| we also set a hidden variable _genre_wikidata to the same values for use in scripts. | ||
| ''' | ||
| PLUGIN_VERSION = '1.4.6' |
There was a problem hiding this comment.
I'd suggest setting this to 1.5, after all this PR also adds a small feature by providing the ~wikidata_genre variable.
Reduce log verbosity, particularly INFO but also DEBUG, and provide additional information.
Refactor code to reduce nesting, primarily using early returns, but also simplifying and generally cleaning.
Fix a few small bugs whose symptoms would probably not be noticed.
I have tested this version, however I don't have not yet found any of my music that has Wikidata genres.