Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support feed item images #41

Merged
merged 14 commits into from Nov 8, 2017
Merged

Support feed item images #41

merged 14 commits into from Nov 8, 2017

Conversation

gmemstr
Copy link
Contributor

@gmemstr gmemstr commented Sep 27, 2017

After some work from myself and florentheobin we've gotten image support for feeds fully done, including tests.

Copy link
Contributor

@kisielk kisielk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, just a bit of clean up needed. Also please fix the gofmt failure in Travis.

README.md Outdated
@@ -1,9 +1,12 @@
## gorilla/feeds
[![GoDoc](https://godoc.org/github.com/gorilla/feeds?status.svg)](https://godoc.org/github.com/gorilla/feeds) [![Build Status](https://travis-ci.org/gorilla/feeds.png?branch=master)](https://travis-ci.org/gorilla/feeds)
## gmemstr/feeds
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably shouldn’t change this...

README.md Outdated

feeds is a web feed generator library for generating RSS, Atom and JSON feeds from Go
applications.

This is a fork by gmemstr which aims to make feeds more "podcast-centered". Essentially making
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be removed

atom.go Outdated
i.Link.Rel = "enclosure"
x.Link = &AtomLink{Href: i.Link.Href, Rel: i.Link.Rel, Type: i.Link.Type, Length: i.Link.Length}
if i.Enclosure != nil && link_rel != "enclosure" {
// if intLength, err := strconv.ParseInt(i.Enclosure.Length, 10, 64); err == nil && intLength > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented or code should be removed

rss.go Outdated
item.Enclosure = &RssEnclosure{Url: i.Link.Href, Type: i.Link.Type, Length: i.Link.Length}
// Define a closure
if i.Enclosure != nil && i.Enclosure.Type != "" && i.Enclosure.Length != "" {
// if intLength, err := strconv.ParseInt(i.Enclosure.Length, 10, 64); err == nil && intLength > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove commented out code

@gmemstr
Copy link
Contributor Author

gmemstr commented Sep 27, 2017

will do shortly - just headee home now. didn't actually expect to make this PR but thanks to florentheobin for the fixed.

@kisielk
Copy link
Contributor

kisielk commented Sep 28, 2017

Thanks.

This changes the API, but I think if we tag a release before, we can probably merge this without feeling too guilty.

@elithrar any thoughts on this one?

@elithrar
Copy link
Contributor

elithrar commented Sep 28, 2017 via email

@gmemstr
Copy link
Contributor Author

gmemstr commented Nov 7, 2017

Bump for this, just saw mux got a new release. Is there anything else I should add to this PR before?

@kisielk
Copy link
Contributor

kisielk commented Nov 7, 2017

I'm ok with it going ahead. @elithrar should we tag 1.0 for the current version of the code, and then 1.1 after merging this? though that doesn't really give people an opportunity to lock to 1.0 in the meantime so not sure if it's worth it since their code will break anyway. Maybe we should just do the bandaid pull approach and merge this and then tag 1.0?

@elithrar
Copy link
Contributor

elithrar commented Nov 7, 2017 via email

@elithrar elithrar merged commit 4b936b5 into gorilla:master Nov 8, 2017
@elithrar
Copy link
Contributor

elithrar commented Nov 8, 2017

@elithrar
Copy link
Contributor

elithrar commented Nov 8, 2017

Thanks @gmemstr and @florenthobein for contributing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants