check config opt import/write before try_write from art.py (fixes #1427)

[reynhout]

No description provided.

[sampsyo]

This can just be if config['import']['write']: (that is, no .get(bool) necessary when used in a condition).

[reynhout]

Heh, sorry. Copied the code from elsewhere in the repo. My python atrophies quickly.

[sampsyo]

There are two possible policies here: embedart when not writing either:

1. just embeds art but writes no other tags
2. does nothing

This change will chooses option 2. That seems fine to me—but if we're going to do that, then we might as well also skip the similarity comparison, the width check, etc., which is all wasted work. So perhaps the plugin should just never call embed_item or embed_album?

[reynhout]

Hmm, good question.

Since --nowrite only prohibits writing to the audio file itself (not the database or art download etc), it seems like it might be reasonable to continue to perform all other operations. This could be useful to verify that the full set of operations meets user expectations before committing the results, and would make it easier to detect failures ahead of time (ImageMagick not installed, etc).

My use model is to run beet import -W and then if I like the results, to write them out with beet write and beet embedart. In that model, I think(?) all of the downloading and transmogrification work is not wasted because the products are saved, and then written to the audio file with the later commands. I haven't tested that fully, but it seems to be accurate for simple cases.

If some of the products would end up being recreated, then that would definitely add up to a lot of wasted work for large imports using --nowrite.

Another question about this PR:

With the current PR, beet embedart (from the command line) will now respect write: no in the config file, despite the explicit intent of the command to do the writing. Should it ignore the config in this case, like beet write does?

I was focused on the case where beet import with plugins: embedart configured would always write tags and art to the file, even when --nowrite or write: no were configured.

[sampsyo]

The work performed on download (i.e., by the fetchart plugin) is separate from what's done here (in beet.art on behalf of embedart). So we can cleanly make that separation and avoid doing the latter work, even when fetching if fully enabled.

With the current PR, beet embedart (from the command line) will now respect write: no in the config file, despite the explicit intent of the command to do the writing. Should it ignore the config in this case, like beet write does?

Aha, good point! Yes. It does seem that the explicit command should always actually write the files.

[sampsyo]

As an additional note: Some tests for the embedart plugin that show these policies being enforced would be really valuable!

[reynhout]

My current fix for beet embedart to force write: yes is just to add config['import']['write'] = True into EmbedCoverArtPlugin() / commands() / embed_func().

(diff: reynhout@f6e0499 )

Is that a reasonable path? It's hard to avoid treating config as a global, and I don't see another mechanism, so please let me know if I've missed something.

I agree on tests, I'll take a look in there too.

[sampsyo]

Thanks for continuing to look into this!

The cleanest way to do this would be to check the flag in the plugin's event handler, not in art.py. That will skip the entire call to the embed functions—and you're right, this will also skip the similarity check and such, but I actually think that's for the best. A "dry run" kind of functionality would be interesting but could be implemented separately.

[reynhout]

Ah yes. That makes it much simpler. Thank you for clarifying.

I'll close this PR and add the updated patch to the issue report. Then take a look at tests.