Skip to content

mediafile: replace with a re-export of beetbox/mediafile#3237

Merged
sampsyo merged 7 commits intobeetbox:masterfrom
arcresu:mediafile
May 31, 2019
Merged

mediafile: replace with a re-export of beetbox/mediafile#3237
sampsyo merged 7 commits intobeetbox:masterfrom
arcresu:mediafile

Conversation

@arcresu
Copy link
Copy Markdown
Member

@arcresu arcresu commented Apr 23, 2019

This re-exports all of the MediaFile package under beets.mediafile. A warning is issued when importing from the deprecated namespace.

Remaining work:

  • Port the outstanding commits from beets.mediafile to MediaFile (Sync changes from beets.mediafile mediafile#6).
  • Make a new release of MediaFile.
  • Update the minimum dependency of beets on MediaFile to that new release.
  • Remove all uses of the deprecated namespace from beets itself (this should be left until later so as to avoid picking up many conflicts in this branch while the previous steps happen).
  • Delete MediaFile wiki page (tracked in Audit the wiki pages #3283).
  • Update docs (e.g. dev/plugins) and wiki.
  • Add changelog entry.
  • Notify downstream packagers about the new dependency (although I don't think that we need to block a beets release on this as discussed in Replace confit with confuse #2061 (comment)).

Part of #1966.

@arcresu arcresu changed the title mediafile: replace with a wrapper around mediafile mediafile: replace with a re-export of beetbox/mediafile Apr 23, 2019
@arcresu
Copy link
Copy Markdown
Member Author

arcresu commented Apr 27, 2019

The following open PRs touch beets/mediafile.py and will need to be (at least partially) migrated to the standalone mediafile. Merge conflicts will appear once this PR is merged.


For future reference the command to build this list (relies on fetching PRs locally) was

git for-each-ref --no-merged=upstream/master refs/remotes/upstream/pr/ --format='%(refname)' | while read r; do if ! git diff --quiet upstream/master...$r -- beets/mediafile.py; then echo ${r##*/}; fi; done | sort -n | sed -e's/^/#/g'

followed by a manual check to remove closed PRs.

@arcresu arcresu added this to the Modularise beets milestone Apr 27, 2019
@arcresu
Copy link
Copy Markdown
Member Author

arcresu commented Apr 29, 2019

I've started adding a new "mediafile" label to issues and PRs that should be completely moved to the standalone MediaFile. I'm not adding it to issues/PRs that affect MediaFile as part of a larger set of changes since these should be split rather than moved.

This was referenced May 19, 2019
@arcresu arcresu marked this pull request as ready for review May 19, 2019 02:08
@arcresu
Copy link
Copy Markdown
Member Author

arcresu commented May 19, 2019

I've gone ahead and finished the MediaFile split so that it's ready to go. I think we should aim to get it merged as early as possible in the current release cycle.

For the list of affected open PRs above, most are outdated and are already conflicting anyway. I left comments on those to alert the original submitters in case they're able to let us know their plans. There are two active PRs that are affected. The author of one is currently splitting and resubmitting the PR, and the other is ~done and I'm happy to manually migrate those changes myself.

One other thing I noticed is that now Mutagen becomes a transitive dependency. It's only used directly by the scrub plugin now. Are we ok with dropping the direct dependency on Mutagen in beets?

@sampsyo
Copy link
Copy Markdown
Member

sampsyo commented May 19, 2019

Yay! I'm very excited about this, and—thanks to your effort ahead of time—the footprint of this change is now fairly small. It will be truly great to have this cleanly factored out.

Yep, I think we can indeed drop the direct Mutagen dependency; it's interesting that scrub is the only thing that really needs to skip the MediaFile abstraction level to do its work.

@arcresu
Copy link
Copy Markdown
Member Author

arcresu commented May 28, 2019

Of the three remaining PRs, one has removed the MediaFile changes and the other two are from branches that have been conflicting for a long time anyway. Shall we hit the button now?

@sampsyo
Copy link
Copy Markdown
Member

sampsyo commented May 29, 2019

Yeah, sounds great! In the spirit of getting releases out the door more quickly, what do you think about pushing out the current release and its handful of fixes and then immediately merging this as the first thing in the next release? We can start the publicity campaign to notify packagers about the new dependency.

@arcresu
Copy link
Copy Markdown
Member Author

arcresu commented May 30, 2019

Alright, so this small release would mostly be for the imagemagick issue? That sounds like a good plan then. Some planning thoughts:

  • The confit/confuse unbundling is also almost ready, mostly waiting on Add helper to interpret items as key-value pairs (port from confit) confuse#50 to go in followed by a release of confuse. Should we aim to do that in the same release (1.4.9 + 1) or do one at a time?
  • The next bit of technical debt I want to tackle is reviving the effort to improve the plugin architecture. I looked through the stalled PR and can see some ways to break it up and make changes that maintain backward compatibility unlike the original approach. Once mediafile and confuse are done that'll be my next focus.

@sampsyo
Copy link
Copy Markdown
Member

sampsyo commented May 30, 2019

Good point! I think doing the Confuse extraction in the same release would be a good idea. It could save people time to add two packages at once.

And I'm really excited that you're interested in helping refactor the plugin infrastructure! This I something I'd really like to clean up, so I'd be really happy to talk about how to approach this. Woohoo! 🚀

@sampsyo sampsyo merged commit 1de894a into beetbox:master May 31, 2019
sampsyo added a commit that referenced this pull request May 31, 2019
mediafile: replace with a re-export of beetbox/mediafile
sampsyo added a commit that referenced this pull request May 31, 2019
@sampsyo
Copy link
Copy Markdown
Member

sampsyo commented May 31, 2019

Done! 🎉 This is the first change of v0.5.0. Thank you again for your work on getting this to happen!!

@GuilhermeHideki
Copy link
Copy Markdown
Member

I used grep to find the external plugins using mediafile:

  • cd the site-packages folder found with python -c "import site; print(site.getsitepackages())"
  • grep -rnw beetsplug -e 'mediafile'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants