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

Use FeedGenerator #2133

Closed
wants to merge 4 commits into from
Closed

Use FeedGenerator #2133

wants to merge 4 commits into from

Conversation

@msnoigrs
Copy link
Contributor

msnoigrs commented Oct 4, 2015

I have just adjusted to work with my feedgenerator for PR.
It use my custom feedgenerator that contains some bugfixes and some enhancements. https://github.com/masayuko/python-feedgen

It's a big patch. please check it.

@msnoigrs msnoigrs changed the title User FeedGenerator Use FeedGenerator Oct 4, 2015
@msnoigrs
Copy link
Contributor Author

msnoigrs commented Oct 4, 2015

I prefer '.xml' to '.atom' for Atom feeds.

@Kwpolska
Copy link
Member

Kwpolska commented Oct 4, 2015

We can’t really change that, and our current Atom implementation is good enough (we want to use feedgenerator for RSS only)

@msnoigrs
Copy link
Contributor Author

msnoigrs commented Oct 4, 2015

OK. I have no problem. I will withdraw this PR.

@msnoigrs msnoigrs closed this Oct 4, 2015
@Kwpolska
Copy link
Member

Kwpolska commented Oct 4, 2015

Wait, no. We could take this from here and make the necessary changes.

@Kwpolska Kwpolska reopened this Oct 4, 2015
@msnoigrs
Copy link
Contributor Author

msnoigrs commented Oct 4, 2015

I know that .atom file extension recommended on RFC 4287 https://www.ietf.org/rfc/rfc4287.txt.
But Google says:

Section 2: Important Checklist
Before you submit your feed, we highly recommend running through the following list to help ensure your file is properly formatted:

  1. Your filename must end with the .xml extension.

at https://support.google.com/merchants/answer/160593?hl=en
So I believe .xml file extension is in advantageous for SEO.

@Kwpolska
Copy link
Member

Kwpolska commented Oct 4, 2015

That’s Google Shopping, not Google Search.

@msnoigrs
Copy link
Contributor Author

msnoigrs commented Oct 4, 2015

Yes. I know. It's my preference.
But I will resubmit to output Atom feeds with .atom extension.

@da2x
Copy link
Contributor

da2x commented Oct 4, 2015

Using .atom will get you the correct Content-Type header from most servers without configuring anything on the server. As a static generator, “without configuring anything on the server” is important. As for “SEO reasons” and client compatibility, the file extension does not matter in the least; only Content-Type – the .atom file extension gets us the correct Content-Type.


# The PubSubHubbub URL for feeds. Defaults to None.
# FEED_PUSH = None

This comment has been minimized.

Copy link
@da2x

da2x Oct 4, 2015

Contributor

You’ve also added FEED_DEFAULT_IMAGE but not added it here nor in nikola.py.

@da2x
Copy link
Contributor

da2x commented Oct 4, 2015

First attempt at building:

TaskError - taskid:render_indexes:en:output/index-atom.xml
PythonAction Error
Traceback (most recent call last):
  File "/usr/lib/python3.4/site-packages/doit/action.py", line 383, in execute
    returned_value = self.py_callable(*self.args, **kwargs)
  File "/usr/lib/python3.4/site-packages/nikola/feedutil.py", line 192, in gen_feed_generator
    fg.title(title=title, type=None, cdata=False)
TypeError: title() got an unexpected keyword argument 'type'
@da2x
Copy link
Contributor

da2x commented Oct 4, 2015

I must be using the wrong feedgen here, because almost none of the features you’re using exist in the feedgen documentation nor do they exist in the code.

@da2x
Copy link
Contributor

da2x commented Oct 4, 2015

👎 You’re stripping away much of the support for RFC 5005. 👎 link rel=next-archive, link rel=prev-archive, link rel=current, and the archive element on stale feeds are all missing. link rel=last have been added but should be removed (as we continuously update with new feeds so there is no last feed).

I can’t get this to run at the moment, but from looking at the code, the output will be quite different what Nikola produces today.

self.site = site

def atom_renderer(self, fg, output_path, atom_path, xsl_stylesheet_href,
pretty=True):

This comment has been minimized.

Copy link
@da2x

da2x Oct 4, 2015

Contributor

This does not need to be an option here nor in rss_renderer(). Always make this pretty, and let users customize the output with filters (like xmlminify).

raise(e)
return data

def gen_uuid(self, path):

This comment has been minimized.

Copy link
@da2x

da2x Oct 4, 2015

Contributor

No. Remove this. The UUID is not recreatable so we cannot produce the same output twice. Use the permalink URIs as IDs as we do today.

Update: Carry on, carry on. I thought even namespaced UUIDs would differ between machines but it doesn’t. Disregard this comment.

This comment has been minimized.

Copy link
@da2x

da2x Oct 4, 2015

Contributor

This should use the full permalinks and not just relative paths. Otherwise all Nikola sites will end up with the same overlapping UUIDs!

Update again: Ah, so you’re already doing that but the variable name is imprecise.

@msnoigrs
Copy link
Contributor Author

msnoigrs commented Oct 5, 2015

I implement 3. Paged Feeds (rel=next|previous|first|last), not 4. Archived Feeds (rel=prev-archive|next-archive|current) on RFC5005.
I think that Paged Feeds is more appropriate for Nikola.

@msnoigrs msnoigrs force-pushed the msnoigrs:feedgen-for-pr branch from 7247abc to 3f31c15 Oct 17, 2015
@msnoigrs
Copy link
Contributor Author

msnoigrs commented Oct 17, 2015

I rewote this PR and test it myself.
Original feedgenerator doesn't have a support of things with RFC5005.
Original feedgenerator author don't react to my PR, so shoud I rename my custom feedgenerator and put to PyPI?

@da2x
Copy link
Contributor

da2x commented Oct 17, 2015

(Having email trouble so didn’t see your reply earlier.) Nikola has archives and the archives have feeds (extremely well suited as sitemaps). The archive feed extension is thus very much suited for Nikola.

@msnoigrs
Copy link
Contributor Author

msnoigrs commented Oct 18, 2015

Sorry for confusing you.
This second PR have a support of 4. Archived Feeds already and my custom feedgenerator have a support of RFC5005.

@msnoigrs
Copy link
Contributor Author

msnoigrs commented Oct 18, 2015

3.  Paged Feeds
...
   Paged feeds are lossy; that is, it is not possible to guarantee that
   clients will be able to reconstruct the contents of the logical feed
   at a particular time.  Entries may be added or changed as the pages
   of the feed are accessed, without the client becoming aware of them.

   Therefore, clients SHOULD NOT present paged feeds as coherent or
   complete, or make assumptions to that effect.

rel=last is no problem.

rel=(first|last|prev|next) are for 3. Paged Feeds in RFC5005.
rel=(current|prev-archive|next-archive) are for 4. Archived Feeds in RFC5005.

@ralsina
Copy link
Member

ralsina commented Nov 1, 2015

@masayuko Sorry about letting your PRs rot ffor so long. I will take a good look at them tomorrow so we can move them forward and merge your work. Thanks for the patience and the effort!

@Kwpolska
Copy link
Member

Kwpolska commented Apr 9, 2020

Unfortunately, this PR has lingered for a very long time, and is probably very outdated (right now, there are 33 merge conflicts), so getting it working with the modern version of Nikola would be a bit of a hassle. I will close this PR and will try to spend some time on re-implementing this feature, partially from scratch, partially using code from this PR.

@Kwpolska Kwpolska closed this Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.