LibriVox / librivox-catalog

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

Add catalog URL to ID3 tags #229

Closed quartertone closed 1 month ago

quartertone commented 1 month ago

This is a proposal to add the catalog URL to the ID3 tags of the mp3 files.

Discussion continued from https://github.com/LibriVox/librivox-catalog/pull/227

(I didn't realize I could just re-branch the upstream master, so I took the circuitous route of resetting my copy of the repo to prior to my commits and then forcing a push... Well, at least now I know how to do that.)

redrun45 commented 1 month ago

(I didn't realize I could just re-branch the upstream master, so I took the circuitous route of resetting my copy of the repo to prior to my commits and then forcing a push... Well, at least now I know how to do that.)

Honestly, I was thinking the re-branch would be easier for you as a first-timer, but that just shows what I know!

Now that you know the git reset and git push --force tricks, you'll be able to update any PR by force-pushing commits to the relevant branch (in this case, quartertone:master), rather than adding new commits on top of the existing ones! This is something I'm still working on improving, myself.

So... if you wouldn't mind a bit of practice... 😆 I think your editor has filled in the blank space to the left of your new code with a bunch of "space" characters. I know some people want tabs, and some want spaces, and there's a whole religious war among the programmer populace over it, but the best thing to do is to match the code that's around. Looking at mixed code in a different editor (or here on GitHub), the lines don't exactly line up. That doesn't keep it from working, but it does look a little funny. 😉

Meanwhile, we'll see if there are more objections I didn't think of, if something else needs to change, and when and just how thoroughly to test this out. I can make no promises on the timeline, but I'll be around!

quartertone commented 1 month ago

Agh, sorry for the multiple pull requests. The first spacing correction fixed the indent, but I forgot about the intervening tab character, which I fixed in the second (third) pull request.

I go back and forth between tab and spaces, depending on my mood. I guess the current spacing format in the repo code (tabs & spaces combined) sort of matches my style haha.

redrun45 commented 1 month ago

I go back and forth between tab and spaces, depending on my mood. I guess the current spacing format in the repo code (tabs & spaces combined) sort of matches my style haha.

Well, the end result looks much better!

But I do think we're confusing two different terms here. We are still on your second "Pull Request". A Pull Request says "hey, I've got this branch or fork over here, will you please copy all of the changes I make there?" You've done that twice - once to make #227, and once to make this thread.

What we're on number three of, is "commits", or snapshots of changes you've made to that branch we're supposed to copy from. As you make more changes to the branch, the list of changes we would be copying with per that same "request" grows longer. If you look near the top of this page, where we're on the "Conversation" tab, you'll see next to it a "Commits (3)" tab, which will show each of those snapshots on the list... where we'd like to have just one snapshot that takes us straight to the correct version.

This is all a long-winded and hopefully educational way to say: whatever you did to close #227 and "start fresh" on this thread, do that again please! It's also entirely possible I'm being too much of a stickler (I don't have much experience with the norms of this kind of thing). What I do know is we've got plenty of time to make this pretty, so it may as well be. Especially if we can learn while we're at it. :wink:

quartertone commented 1 month ago

Right! i meant "commit". Still learning the terminology. I will send up a fresh clean pull request to keep it nice and tidy.

Thanks for your patience and the education!