search

eduncan911 / podcast

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

Mixup between enclosure.length and itunes:duration #3

Closed naglis closed 7 years ago

naglis commented 7 years ago

Hi @eduncan911,

first of all, thanks for the work you've put into building this library.

I've been working with it and I think there is a slight mixup between enclosure.length attribute and itunes:duration tag. According to RSS 2.0 spec and iTunes podcast spec, the length attribute of enclosure should be the size of the file in bytes. However, the AddEnclosure method signature takes an argument called lengthInSeconds, which I guess is misleading. Moreover, the Podcast.AddItem method set's the newly added Item's IDuration (which should be duration in as in length of time) by using the Item's Enclosure.Length.

My suggestions/questions:

What is your opinion?

eduncan911 commented 7 years ago

Awesome @naglis . However did I miss that in the spec, as I must have read through/referenced it a dozen times?!

You are exactly right of course. 

// must be represented as Bytes, not length.
Item.Enclosure.Length

// Item.AddEnclosure() signature must change to accept lengthInSeconds and lengthInBytes.
// I'll review again to see which is required and which is not.
Item.AddEnclosure()

That AddEnclosure() will be a breaking change though... Guess that will require a bump in the major version to v2.x.

As for Item struct missing additional field... Let me evaluate that a bit more. I think I had everything already; but, I may have missed it.

I should be able to take care of all of these in the next day or two.

eduncan911 commented 7 years ago

@naglis what about just changing AddEnclosure from:

func (i *Item) AddEnclosure(url string, enclosureType EnclosureType, lengthInSeconds int64)

to:

func (i *Item) AddEnclosure(url string, enclosureType EnclosureType, lengthInBytes int64)

This keeps prevents break changes to the existing API and I can keep it in the 1.x releases.

The downside being the API wouldn't have that 4th field for "durationInSecond". Also, item.enclosure.itunes:duration is not a required field. I don't think this is a bad thing though because that IDuration field has all sorts of formatting options from raw total seconds to HH:MM:SS. Leaving it out of the API allows the end user to set as they want.

I'll go ahead and put this together. But let me know your thoughts.

eduncan911 commented 7 years ago

@naglis added PR #4

give it a look see for your approval.

note that I added an Item.AddDuration instead of modifying the Item.AddEnclosure as the itunes:duration is not a required field. this made more sense to me.

...and, it allows me to strongly format itunes:duration into a nice HH:MM:SS format later on.

naglis commented 7 years ago

Hi @eduncan911,

wow, thanks for you quick response.

Your changes in #4 look good to me 👍

eduncan911 commented 7 years ago

coolio. squashed and pushed onto master. 👍

eduncan911 commented 4 years ago

@naglis Btw #8 added the HH:MM:SS formatting of Duration a bit ago.