eduncan911 / podcast

iTunes and RSS 2.0 Podcast Generator in Golang
MIT License
132 stars 35 forks source link

Allow providing GUID on Podcast #15

Closed damjanek closed 4 years ago

damjanek commented 5 years ago

This MR allows user to supply his GUID (which is kinda crutial if you're reworking some existing podcast).

eduncan911 commented 5 years ago

First, thank you for the PR!

I'll research this a bit more; however, I seem to remember an Apple Podcast requirement that they match - or are the same (meaning, your existing feed may be invalid if not). If you can point me to the Apple Docs that it allows GUID separate from the URL, or defined the GUID as an actual ID, I'll merge it. I could be completely and utterly wrong (it's known to happen).

(Apple's official podcast specs/ref is linked in the README)

Also, there are no tests here for the new logic.


EDIT

Another argument could be made that this PR continues to enable the "not restrictioned" promise I made in the project, to where you are free to form the podcast anyway you want. Which in that case, I'm more than happy to merge (please add some tests though!).

However, we loose some of that promise in the 2.0 branch where I am making this package two-way compatible with marshalling both ways. I actually have open questions there for the community in PR https://github.com/eduncan911/podcast/pull/5

damjanek commented 5 years ago

Hey Eric,

Quoting the docs (https://help.apple.com/itc/podcasts_connect/#/itcb54353390):

Globally unique identifiers (GUID) are case-sensitive strings. If a GUID is not provided an episode’s enclosure URL will be used instead. If a GUID is not provided, make sure that an episode’s enclosure URL is unique and never changes

So it's gonna be enclosure url if guid is not provided. It does not state that it has to be exactly the same. About the tests - I'll take care of that.

Thanks, Damian

eduncan911 commented 5 years ago

Sounds good!

Though, they do state GUID. Should we enforce it to be an actual GUID/UUID?

damjanek commented 5 years ago

We shouldn't. When you expand their GUID definition in the docs it states: „A unique, case sensitive, reference number for each item in a collection. A GUID can be the source URL, the RSS feed title, a serial number, or any other unique number. For example, https://cupertinouniversity.edu-dz/collection1.mp3. GUIDs only need to be unique within each RSS feed.”

So given this - we cannot enforce the format of GUID. It's up to the user to decide.

On Thu, Oct 3, 2019 at 3:17 PM Eric Duncan notifications@github.com wrote:

Sounds good!

Though, they do state GUID. Should we enforce it to be an actual GUID/UUID?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/eduncan911/podcast/pull/15?email_source=notifications&email_token=AAMBLZL6KRH4WUKUBPZQUADQMXWHXA5CNFSM4I5CNDK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAIFLPQ#issuecomment-537941438, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMBLZJ4BQTLZD3IJFWT2NTQMXWHXANCNFSM4I5CNDKQ .

--

Damian Szeluga

eduncan911 commented 5 years ago

OT: I need to convert this repo over to GitHub Tasks (to fix the CI).

For now if you can paste the output of go test -cover showing 100%, I'll merge it.

damjanek commented 5 years ago

`@dszeluga-mbp (~/repos/podcast) $ git log | head commit 880c6a7d76fbb076ba143a1c12358c2ed61bf1a0 Author: Damian Szeluga damian.szeluga@gmail.com Date: Thu Oct 3 15:37:11 2019 +0200

Adding test for user supplied GUID

commit 572245022e586c2fccbb5eff40855744bb3e45ef Author: Damian Szeluga damian.szeluga@gmail.com Date: Thu Oct 3 14:24:46 2019 +0200

@dszeluga-mbp (~/repos/podcast) $ go test -cover PASS coverage: 20.1% of statements ok _/Users/dszeluga/repos/podcast 0.084s`

eduncan911 commented 5 years ago

Humm. That doesn't seem right.

Let me find some time to pull down the repo/branch and test.

eduncan911 commented 5 years ago

I'm working on the new Ci with GitHub Actions now.

If you don't mind, I'd like to keep this PR open to test if that's ok?

damjanek commented 5 years ago

Sure, no rush.

coveralls commented 4 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling ece2d1a08600c075aaafecd10ce04db2a5157101 on damjanek:master into 10213c61cee52248f7651b28827b83133aa447be on eduncan911:master.

damjanek commented 4 years ago

Is there a chance to revisit this MR?

eduncan911 commented 4 years ago

thanks for fixing gocoveralls!

however, when i run go test on your branch, i get a lot of failed tests...

It looks like gocoveralls is only running a test against that one commit, not this entire branch oddly enough.