acousticbrainz: refactor plugin #2251
Conversation
|
can you split it into more commits then? make it much easier to review. |
|
I probably should :). I just liked this solution very much and because there was no tests to look over my shoulder I wanted to get feedback as I go on (Also it was 3AM). |
|
I split it up in two commits, that's really the best I could do. |
| data = self._get_data(item.mb_trackid) | ||
| if data: | ||
| # Get each field and assign it on the item. | ||
| item.danceable = get_value( |
There was a problem hiding this comment.
I'm sorry I tried my best to convince git this whole patch did not change as much as git thinks it did, but to no avail.
There was a problem hiding this comment.
Nice! This looks good to me so far—I very much like this declarative approach better than the old imperative one. I haven't actually tried running it, but I say we merge it if it seems to work after a little bit of experimentation. (And then, of course, we should try to add some end-to-end tests using captured AB service responses.)
| def _fetch_info(self, items, write): | ||
| """Get data from AcousticBrainz for the items. | ||
| """ | ||
| for item in (item for item in items if item.mb_trackid): |
There was a problem hiding this comment.
For some reason, this strikes me as slightly less idiomatic than:
for item in items:
if not item.mb_trackid:
continuebut I definitely don't have a strong argument against it.
There was a problem hiding this comment.
I did not like it either when I wrote it, I think I will go back on it.
| if write: | ||
| item.try_write() | ||
|
|
||
| def _map_dict_to_scheme(self, dictionary, scheme): |
There was a problem hiding this comment.
It goes without saying, but these helpers (this one and the one below) could use some nice docstrings to explain how they fit together.
There was a problem hiding this comment.
Yes I plan for all of these to have docstrings, but this one especially is hard to get right!
|
The bulk of the doc is in! Let me know what you think. I'm continuing to do some rudimentary tests on my machine, but if anyone here uses this plugin extensively or knows its potential quirks, it would be nice to have some cross checks or results. |
|
I'm writing unit tests and there are remaining obvious blunders in my code. Hopefully I'll fix those and then get out an integration test as well. |
|
I added a test with realistic data for the business logic, and the validation data was generated using the plugin on master. So far so good. I think I just need to get an integration test out now. |
nathdwek
changed the title
|
Two questions:
|
nathdwek
force-pushed
the
drop-reduce
branch
from
f296e3d
to
5523702
Compare
|
I took a step towards simplifying the approach a bit by dropping |
* Less lazy names * Separate root function from childs more naturally * Actually use defaultdict
* I don't see DefaultList be really helpful in many other cases, so having a beets.util.collections module (which could also conflict with the collections module from standard library) with only that in it is a little silly. * It was elegant and concise, but there are implementation issues: it is not recommended to subclass builtin types, but the alternatives differ between python 2 and 3 (subclass sequence or container or collections.abc?), moreover, interpreters can differ in the way they map syntaxic sugar to magic functions. * Also, slicing and negative indexing could do weird things, so the class wouldn't be really intuitive and simple at the same time
nathdwek
force-pushed
the
drop-reduce
branch
from
5523702
to
eb1ea61
Compare
* Readability and consistency * Better logging: warn by default for attributes that were not found, but don't drown user with info when everything is going well.
|
Here it comes. |
Personally I like this rewrite very much. However, the rewrite is sizeable, there is no test coverage and I do not even use this plugin, so we should think twice about it before merging.
Summary of changes:
reduceandoperatoralthough they were not outrageously misused