search

geigerzaehler / beets-alternatives

Beets plugin to manage external files
MIT License
91 stars 21 forks source link

Replace deprecated imports of beets submodules by the new standalone packages #63

Closed wisp3rwind closed 1 year ago

wisp3rwind commented 3 years ago

The relevant changes in beets will actually only land in beets>1.4.9. Thus, if and when we merge this, the version requirement for beets also needs to be bumped.

wisp3rwind commented 3 years ago

Makes sense. We should probably add mediafile and confuse as explicit dependencies.

Do we need to? These are integral parts of beets itself, there's no way these can be missing.

We should also make this a major version change (i.e. to 0.11) if we require beets v1.4.9.

Yes, bumping our major version in that case sounds sensible.

We don't really require beets >1.4.9, though. Only the tests depend on the import location, none of the modules has changed, though. So maybe, I should change this PR to check beets' version and do conditional imports. I dislike that idea, but it might be warranted. OTOH, there's probably no good reason for anyone not to upgrade to beets 1.5.0 once it's released. Do you have a clearer preference on how to handle this.

geigerzaehler commented 3 years ago

I totally missed that we only require these for dependencies :facepalm:. We can make mediafile and confuse explicit test dependencies with test_requires. Then the tests with beets 1.4.7 should pass. If this does not work, then I’m ok with bumping the requirement to 1.4.9.

lovesegfault commented 3 years ago

For what it's worth, I think most beets users at this point just run some rev of master as the releases rarely if ever happen. It would be nice to have this merged and just direct users who want to remain on 1.4.9 to the last release.

wisp3rwind commented 3 years ago

For what it's worth, I think most beets users at this point just run some rev of master as the releases rarely if ever happen. It would be nice to have this merged and just direct users who want to remain on 1.4.9 to the last release.

I tend to disagree: IIRC, this only leads to warnings when running the tests, and is not otherwise causing any issues, so we may as well postpone merging a bit longer. Or is there any actual situation where these warnings are a problem?

wisp3rwind commented 1 year ago

This should certainly be merged with the updated version requirement in #70.

geigerzaehler commented 1 year ago

This should certainly be merged with the updated version requirement in #70.

@wisp3rwind Would you like to update this PR and I’ll give it another look?