hook: switch to using check_call

[jackwilsdon]

A small change but hopefully part of a larger piece of work around #2835.

[sampsyo]

Seems right to me! Maybe we should also catch CalledProcessError and print a friendly message when the hook fails?

[jackwilsdon]

Aha, good catch! I clearly didn't look into the check_call documentation enough to see it can throw things other than an OSError 😬

[sampsyo]

Nice; this looks perfect to me! Thanks for adding tests!

TBH, this might address 90% of the real-world utility of #2835. As you noted in that thread, it's not entirely clear how we'd actually go about implementing on_error as proposed. Actions like skip_album would only even make sense for a subset of events, and even then, the infrastructure is not set up yet to let events communicate such signals back to the import pipeline.

So I think it might be best to merge this (with a changelog entry!) now and leave any consideration of user-defined error handling for another day.

Thank you again for addressing this!

[jackwilsdon]

I've gotten half way through implementing support for on_error - I've implemented on_error: abort by raising a ui.UserError which does seem to quit beets, although I'm not too sure on what state it actually leaves beets in 😕 I may as well go ahead and finish it off a bit more (thinking of just implementing abort, log and ignore without any of the skip_* handlers) and I'll open a PR for it so we can discuss the merits of implementing it there.

I'll go ahead and update the changelog (I always seem to forget this step!).

[sampsyo]

Perfect; thanks! I merged this as is.

That's cool; thanks for looking into on_error. And indeed, I'm actually also not entirely sure how dangerous just raising a UserError might be—it seems to depend a lot on the context, such as the particular event that is aborting things? Maybe we can get feedback from the original requester about what exactly they wanted to happen and why…