Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

embedart plugin disregards "write: no" and "-W" config opts #1427

Closed
reynhout opened this issue Apr 17, 2015 · 6 comments · Fixed by #1435
Closed

embedart plugin disregards "write: no" and "-W" config opts #1427

reynhout opened this issue Apr 17, 2015 · 6 comments · Fixed by #1435
Labels
bug bugs that are confirmed and actionable

Comments

@reynhout
Copy link
Contributor

Any import action with embedart enabled causes a full write of tags and art, even when config file specifies write: no, or commandline includes -W.

CAUSE: art.py does not check config option import/write before calling try_write.

SUGGESTED FIX: check config option first! :) PR will be attached presently.

@reynhout
Copy link
Contributor Author

It might be preferable to add the config check into library.py function try_write itself.

There are two other locations that call try_write: importer.py/manipulate_files and library.py/try_sync. These two check config settings properly.

PR #1428 adds the config test to art.py/embed_item, for a third occurrence of similar code.

@sampsyo sampsyo added the bug bugs that are confirmed and actionable label Apr 17, 2015
@sampsyo
Copy link
Member

sampsyo commented Apr 17, 2015

Thanks for the report, and for the fix! This is indeed a problem.

@reynhout
Copy link
Contributor Author

A better fix, hopefully in the correct place:

reynhout@febcef1

@sampsyo
Copy link
Member

sampsyo commented Apr 21, 2015

That looks awesome; thanks. If you can just add a quick note to the changelog about the fix, this will be ready to merge. 😃

@reynhout
Copy link
Contributor Author

OK, will do.

How should I add a test the embedart plugin? Should it go into test_importer or test_embedart? Should it be its own class (EmbedartPluginTest?)

I don't see code I could reuse for testing plugins from the import CLI command. Looks like I'd need to reproduce the setup and teardown bits into a new class. Is there a simpler way? Thanks!

@sampsyo
Copy link
Member

sampsyo commented Apr 23, 2015

Great! If you open a new PR with the new change, we can merge that now.

Then the tests can come later. As you have inferred, they will be a little involved. Some setup and teardown will indeed be needed—some of the other plugin tests (e.g., test_fetchart.py) can provide a template.

reynhout added a commit to reynhout/beets that referenced this issue Apr 23, 2015
reynhout added a commit to reynhout/beets that referenced this issue Apr 23, 2015
sampsyo added a commit that referenced this issue Apr 23, 2015
…_reynhout

Fix #1427: embedart plugin should observe write config
reynhout added a commit to reynhout/beets that referenced this issue Apr 24, 2015
* upstream/master:
  Skip symlink tests on Windows
  Provide Unicode to Jellyfish 0.5.0
  Slightly expand changelog for beetbox#1436
  Fix autotag test: duplicated assertion
  Update changelog for beetbox#1427 fix
  Test config opt import/write before embedding art (fixes beetbox#1427)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs that are confirmed and actionable
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants