LibriVox / librivox-catalog

LibriVox catalog and reader workflow application
https://librivox.org
MIT License
36 stars 17 forks source link

Adding reader IDs to mp3 id3 tags #227

Closed quartertone closed 1 month ago

quartertone commented 1 month ago

https://forum.librivox.org/viewtopic.php?p=2330790#p2330790

redrun45 commented 1 month ago

To save some time in reading the thread, this is still in the early discussion phase.

Points on this code as currently written:

Again, early discussion! But this serves as a heads-up, at least. 😉

twinkietoes-on commented 1 month ago

This will need MAJOR checking on the staging server before making it live. Changing something in the cataloging process could have unintended consequences.

I'm not 100% sure this is wanted; just because one or two people want it doesn't mean it should be changed. Some readers might not want it.

redrun45 commented 1 month ago

That does remind me: I've got a patch that makes it easy to publish dummy projects. They go to IA's tests collection, which is periodically cleared out.

I still need to get the patch ready as two PRs (one for code and one for Ansible setup), but I'll bump that up my list. If/when we have cataloging changes like this one ready for testing on the staging server, it will be nice to do that without needing to clean up dummy projects afterwards.

quartertone commented 1 month ago

I sent a new pull request reverting the previous changes, and pivoting to adding the catalog URL instead. Hopefully this will be less objectionable.

redrun45 commented 1 month ago

Sorry, I'm not as careful in explaining terms as I ought to be. You did say this was your first PR!

We'd like to have this particular change as a Pull Request containing only one commit. If you fork the LibriVox master branch again, add in just the change we're looking for at the moment, and save that as a new branch, you'll be able to open a new, "clean" Pull Request from that branch.

We'll quit this PR one without saving, and continue discussion on the new one. That way, if/when we're ready to "save" (or, as the term is, "merge") that new one into the project, there won't be any code in the project's history that we've never actually used.

Edit to add: in case that term "objectionable" wasn't tongue-in-cheek, I want to say: thanks for putting the work on this, even if we can't guarantee it makes it in. I very much appreciate the time and effort on your part, but we also need to make sure it's an improvement that's worth any draw-backs, or at least the effort of making sure there are none. 😉

quartertone commented 1 month ago

Ah, got it! I'm reading up about branches and pull requests. I'll submit a clean PR once I figure it out.