Rewrite the clearart method in order to send write events to the plugins.

[nathdwek]

Currently, beet clearart doesn't notify plugins that some files are modified. This can be a problem when for example using the beets-check plugin because checksums aren't recomputed and the modified files are then detected as not intact during subsequent operations.

I finally settled on fixing this by using the try_write method which allows to simplify the code and centralizes the event sending so that this functionality isn't left out in the future. However, by doing that, beet clearart doesn't delete the image tag, but just erases its value. I don't know if this could be an issue.

The beets-check plugin is the only one I know which needs to be aware of a beet clearart operation so I don't know if I can write tests for this. Can external plugins be called in the test suite?

[sampsyo]

Cool! Thank you for simultaneously simplifying the code while making it more correct. ✨

A test for this could just assert that the appropriate event gets sent (using a dummy plugin) when the art is cleared. Would that work?

It would still be nice to remove the art tag altogether. Maybe we could refine the interface for Item.write and Item.try_write so that any Nones in the tags dict mean "remove this tag from the file"? Or some other way of specifying some tags to remove during the write.

[nathdwek]

Thank you for your input! I will try to come up with a test soon.

I also looked at how tags could be removed from the file with the write method, and it seems that this is actually already the case, with the exact same convention - None means delete*, so everything is already all good in that regard.

*See: https://github.com/sampsyo/beets/blob/master/beets/mediafile.py#L1502

[sampsyo]

Nice! That should do it. Thanks for sorting that out.

[nathdwek]

I added 2 commits with a first shot at testing event sending.

However, the test suite fails, here's the output for a single test:

FAIL: test_delete_art (test.test_mediafile.WavpackTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/nathdwek/opt/beets/test/test_mediafile.py", line 92, in test_delete_art
    self.assertEqual(eventConsumer.popEvent("write"), True)
AssertionError: False != True
-------------------- >> begin captured logging << --------------------
beets: DEBUG: Sending event: write
beets: DEBUG: Traceback (most recent call last):
  File "/home/nathdwek/opt/beets/beets/mediafile.py", line 1349, in __init__
    self.mgfile = mutagen.File(path)
  File "build/bdist.linux-x86_64/egg/mutagen/_file.py", line 222, in File
    fileobj = open(filename, "rb")
TypeError: coercing to Unicode: need string or buffer, NoneType found

beets: ERROR: uncaught Mutagen exception in open: coercing to Unicode: need string or buffer, NoneType found
beets: ERROR: error reading None: None: coercing to Unicode: need string or buffer, NoneType found
--------------------- >> end captured logging << ---------------------

I'm willing to investigate the assertion fail, but I've got no idea on how to take the mutagen errors. (Regarding the assertion error, the previous fix works on my machine (TM) with regular, non dummy files).

I'll probably still try to tackle this anyway tomorrow or in the rest of the week, but any help with mutagen and fixtures would be appreciated. 🙏

[sampsyo]

That's certainly odd. I don't quite see what's going wrong here, but I do think you might be testing at the wrong level. You want to test the beets.art.clear() function, which is actually at a different level of abstraction from MediaFile. Testing there won't trigger your new code.

Perhaps you want to put a test in test_art.py or test_embedart.py, in the approximate style of ArtSimilarityTest?

The event-capturing plugin fixture looks good, though!

[nathdwek]

Oh you're totally right. I'll give it another try.

[nathdwek]

Just a quick update: I've got some family stuff getting in the way of writing the tests and those are proving to be a little more challenging then expected, but I should eventually be able to sort this out.

[sampsyo]

Awesome! Thanks for sticking with it.

[nathdwek]

I'm sorry but I really can't find a way to write a good test for this. One of the main problem is that I struggle to correctly load a dummy plugin if it doesn't reside in beetsplug/, and I don't really like putting test code in there. Even then, my best attempt that involves a test plugin residing in this folder still feels really chunky and inelegant.

Do you have a real interest/need to thoroughly test the plugin observer pattern? (Is it worth refactoring the code and putting in place the infrastructure to do this?) If so, could you share some leads because I'm seriously hitting a wall now.

[sampsyo]

No worries! Let's merge without a test for now and, later, work out the details. If you're interested, we can cook up a clean strategy in a new issue.

In the mean time, would you mind rolling back the current test code? With that and a changelog entry, we can merge now.

[nathdwek]

All right let me finalize this then. Do you prefer history rewrite or revert commits?

[nathdwek]

I went with rewriting history and used this as an opportunity to cleanly rebase on top of master (also because I wanted to try rebasing rather than merging as a way to contribute.). I hope it's ok for you.

Also I took care of the changelog. I didn't put a reference to the embedart plugin at the beginning because I think the fix happened at the core level, I don't know if it's how you'd assess it.

It makes me a bit uncomfortable plugging in my name like that in the changelog 😊. but at least now hopefully you'll juste have to presse the big red merge button. Again, I hope it's ok this way for you.

As always thank you for your responsiveness. I kept the raw commits of my experimentation with tests on another branch, I will probably try to have a fresh look at this some day.

[sampsyo]

This looks perfect! Rewriting the history is just fine (for the record, either way is cool with me). And you're absolutely right to thank yourself; you deserve at least that. 😃