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

Jump to bottom

Use the summary as the feed's summary #1989

Closed
wants to merge 4 commits into from
Closed

Use the summary as the feed's summary #1989

wants to merge 4 commits into from

Conversation

eevee
Copy link
Contributor

@eevee eevee commented Aug 8, 2016 •
edited

This fixes #1886. I tried this on my own blog and the results are definitely more pleasing: <summary> contains a summary, <content> contains the full content.

As mentioned in that issue, this does change the behavior for RSS. RSS's <description> is intended as a summary, and it has no equivalent to <content>, so with this change, RSS feeds won't contain the full content any more.

I can add an option to force putting the full content in the summary/description tag, if this is a problem. Alternatively, just use Atom. ;)

(For what it's worth, I checked my new feed in feedly, and it appears to understand <content> and still show the full article. I don't know how other readers behave.)

eevee added 3 commits Aug 8, 2016
@eevee
Improve tests' git-diff output on Python 3, a bit
ddf8e06
@eevee
Use the summary as the feed's description, and put content in content
2004d0a 
This adheres more closely to the specs, especially Atom, where the
'description' arg becomes <summary>.

Note that this means full article content will no longer appear in RSS
feeds.
@eevee
Whoops; fix the fr_FR feeds too.
4a0e376
@iKevinY
Copy link
Member

iKevinY commented Aug 17, 2016

Looks good to me; can we get this merged in, @justinmayer?

@ingwinlu
Copy link
Contributor

ingwinlu commented Aug 17, 2016

lgtm

@justinmayer
Copy link
Member

justinmayer commented Sep 11, 2016

I just ran python -m unittest discover and am seeing three failing tests on current master (d6ac93a), with at least one failure being seemingly related to <updated> tags being expected but absent in the output. I'm seeing similar failures when running on the code from this PR. This is with FeedGenerator 1.9 on Mac OS 10.11.6 and Python 2.7.11. Anyone else seeing this and/or have any insight into what's causing them?

Just thought it would be good to resolve this before proceeding. (^_^)

@iKevinY
Copy link
Member

iKevinY commented Sep 12, 2016 •
edited

Anyone else seeing this and/or have any insight into what's causing them?

Hmm, I'm also seeing test failures for the current master: four when running python -m unittest discover and three when running nose. (feedgenerator 1.7, Python 2.7.10, OS X 10.11.6)

iKevinY
iKevinY approved these changes Sep 16, 2016
View changes
Copy link
Member

@iKevinY iKevinY left a comment

Looks good to merge after resolving conflicts against the current master.

@Scheirle Scheirle mentioned this pull request Sep 25, 2016
Merged
4 tasks
@eevee
Copy link
Contributor Author

eevee commented Sep 28, 2016

Merged with master.

The test_error_on_warning test is now failing on my machine, but it's also failing on master, and Travis seems happy. Odd.

@iKevinY
Copy link
Member

iKevinY commented Sep 29, 2016

I noticed failing tests on my machine too, but I updated my dependencies and everything seems to pass now (when running python -m unittest discover at least). I think it might have to do with the fact that Pygments is pinned to 2.1.3 now?

@avaris
Copy link
Member

avaris commented Sep 30, 2016 •
edited

About test_error_on_warning, see: #2007

Test runner in unittest is annoyingly overriding warning filters, so you need the -Wd flag to stop that.

Other test runners don't have that issue. Hence the happy travis (which uses nose instead).

@iKevinY
Copy link
Member

iKevinY commented Sep 30, 2016

Interestingly, I do get failures with nose, but not with the base python -m unittest discover.

iKevinY
iKevinY approved these changes Oct 1, 2016
View changes
@justinmayer
Copy link
Member

justinmayer commented Oct 22, 2016

Merged via eac8fa2. Many thanks to @eevee for working on this and to all the folks who helped review.

@Totktonada Totktonada mentioned this pull request Nov 15, 2016
Merged
Totktonada added a commit to whatifrussian/website that referenced this pull request Nov 21, 2016
@Totktonada
Add options to handle Pelican's RSS changes
b770bf0 
It gives back full content in RSS feeds. This was changed to
summary-only in [1]. The option was added in [2]. Both between 3.6.3 and
3.7.

The only changes in feeds between 3.6.3 (with hardcoded html5 as
markdown's output format) and 3.7 are the following:

* Atom feeds: full content of article moved from <summary/> to
  </content> and <summary/> becomes really summary.
* Atom/RSS feeds: strings in <id/> / <guid/> slightly changed by [3].

The first should be good, the second can cause some RSS/Atom readers to
find fake new items when the website will updated.

[1]: getpelican/pelican#1989
[2]: getpelican/pelican#2051
[3]: getpelican/pelican#1306
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iKevinY iKevinY

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Atom feed should output both <summary> and <content>
5 participants
@eevee @iKevinY @ingwinlu @justinmayer @avaris