[Question] Make the podcast.New() smarter?

[eduncan911]

I didn't expect people to find this repo any time soon and I see a number of people already using it. Oops. Hehe.

There is a breaking change I wanted to do; but, I am holding off because of others that may be using it.

I want to change the podcast.New() method because of a number of reasons:

• Make it simpler (don't ask for datetimes, use helper methods for them)
• add Validation
• with Validation, comes error handling

Basically, I want to change this:

func New(title, link, description string, pubDate, lastBuildDate *time.Time) Podcast

To this:

func New(title, link, description string) (Podcast, error)

And add these:

func (p *Podcast) AddPublishedDate(t time.Time) func (p *Podcast) AddLastBuildDate(t time.Time)

Notice that Podcast.New() no longer will take in time.Time, and now also returns an error if validation isn't correct (e.g. missing one of title, link or description).

Breaking Change This would obviously be a breaking change to anyone currently using it (and not vendoring). Therefore, it would require a version bump to 2.x.x.

IMO
I'm on the fence about this. I like APIs that allow package.New() without requiring me to check for errors. They should guarantee a result with package.New() that just works. That was my originally intention when I created the New() method.

But at the same time, how do you know your Podcast is valid if we don't have validation checks? The answer to that is simple: Apple rejects your podcast when you submit it for not being valid.

---

Update 17/02/16: Already added the two new funcs. Just need to discuss the New() question now.

[eduncan911]

Added the following in commit cd785c6 for v1.2.0.

func (p *Podcast) AddPublishedDate(t time.Time)
func (p *Podcast) AddLastBuildDate(t time.Time)

Question still stands as to rather do the breaking change or not for New().

[eduncan911]

@naglis since u are the only other one I've heard using this shiny new package, could you review the question above and provide your opinion?

[naglis]

Hi @eduncan911,

IIUC according to RSS 2.0 spec both published date and last build date are optional, so it makes sense to not require people to set it when using New(). So I guess this comes down to the question of whether to do the breaking change now and have to support both v1 and v2 branches?

(Subjective opinion ahead) Personally, I don't mind having to set these two additional fields in New that much and if I were a maintainer of a library, I would maybe wait for more "breaking stuff" to accumulate before doing the breaking version jump.

[eduncan911]

Going to call this package final for now. Maybe if someone finds this closed issue in some distant future, we can address a 2.0 release.

Until then, enjoy!