Fix for issue #1693 (Apply data_source field to albums)

[apitofme]

This adds the data_source field (flex-attribute) to Albums during an import, it copies the value from the first Item's corresponding field.

Does this need a conditional test to make sure that the Item field exists and actually has a value first?

[sampsyo]

How about self.album.data_source = ... instead of using the set_parse method?

[sampsyo]

Thanks for getting this started! Indeed, it does need a check like that to avoid an error when the field is missing. You can see this by running the tests if you want!

[apitofme]

sampsyo commented 2 hours ago
... You can see this by running the tests if you want!

Just a 'curiosity' from running the tests... In addition to the extra Python modules required to run the tests (as listed in setup.py) I got errors reporting that confuse and mediafile were missing.
(I thought they were installed as dependencies to beets?!)

Anyway, other than that I just had to comment out the custom output templates in my config file.

There was also this error output:

======================================================================
ERROR: test_relative_to (test.test_play.PlayPluginTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "x:\development\python\venv\lib\site-packages\mock\mock.py", line 1330, in patched
    return func(*args, **keywargs)
  File "X:\Development\Python\beets\test\test_play.py", line 80, in test_relative_to
    path = os.path.relpath(self.item.path, b'/something')
  File "x:\development\python\venv\lib\ntpath.py", line 562, in relpath
    path_drive, start_drive))
ValueError: path is on mount b'C:', start on mount b'X:'
-------------------- >> begin captured logging << --------------------

which didn't seem important but does perhaps indicate that the test needs to be modified to allow for relative paths (?) starting from mount points other than drive C: ?

[sampsyo]

I got errors reporting that confuse and mediafile were missing.
(I thought they were installed as dependencies to beets?!)

Yes, they should indeed be! Maybe a reinstall with pip install -e . would fix it?

which didn't seem important but does perhaps indicate that the test needs to be modified to allow for relative paths (?) starting from mount points other than drive C: ?

Hmm; interesting! I don't quite see the problem yet, but if you're interested in helping out, perhaps you could try fixing this test in a separate PR! It is, however, suspicious that the test doesn't run into a problem on AppVeyor?

[apitofme]

Maybe a reinstall with pip install -e . would fix it?

I just installed them separately with pip

It is, however, suspicious that the test doesn't run into a problem on AppVeyor?

It's probably only an issue with my particular set-up

...

Anyway I'm glad that this PR passes the tests now.
Thanks!

[sampsyo]

I think a simpler expression 'data_source' in self.imported_items()[0] should work… any chance you could give that a quick try?

[apitofme]

No worries, all done
...I must admit I had wondered if it was possible that way, but with my inexperience and in my haste I neglected to test it first. Thanks again :)

[sampsyo]

Great! If this indeed seems to fix the problem in your tests, can you please add a quick changeling entry and we'll call this done? ✨

[apitofme]

Changelog updated - since the original request was tagged as a 'feature' I added it under 'New Features'
... I assume/hope that's right (although it feels like a bug-fix!)

[sampsyo]

Looks great; thank you! Just to double check, have you tried importing an album with this to confirm that the fix actually works and the album gets the data source field as intended? If so, then we're good to go!

[apitofme]

I did indeed test it, though hardly extensively ... I only tested it with one album import :/

Basically as per the evidence I submitted during the work building towards this PR, I would simply remove an existing album from beets (in this case "How to Destroy Angels") and import it again. I would then perform a search, first for items then for the album, with the data_source field included in the output for comparison -- I did this each time I made changes to the code for this feature.

[sampsyo]

Perfect! Thanks for confirming. And thanks for your work on this!