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

Validation fixes #26

Merged
merged 3 commits into from
Sep 16, 2014
Merged

Validation fixes #26

merged 3 commits into from
Sep 16, 2014

Conversation

download13
Copy link
Contributor

These are fixes for the Atom output function.

Added the ability to specify an id element in the feed and entries. Also removed the feed -> link element with no rel. I tested the output against the feed validator and it seems to go through fine now.

jpmonette added a commit that referenced this pull request Sep 16, 2014
@jpmonette jpmonette merged commit 1e80b3d into jpmonette:master Sep 16, 2014
@ArtskydJ
Copy link
Contributor

@download13 @jpmonette what exactly is id? Should I just generate a random uuid? Should I use link?

Ideally, id should be documented.

@download13
Copy link
Contributor Author

Yes, a uuid is the customary way to do it. Some people use the article permalink as the id but this is generally considered a bad idea.

I'm not sure if there's user-freindly documentation about it, but you can find the relevant first-source here.

atom:feed elements MUST contain exactly one atom:id element.

@ArtskydJ
Copy link
Contributor

A different id for each item? So if I have 2 posts, I'll generate 2 ids?

@@ -212,7 +216,7 @@ function atom_1_0(options) {

var entry = [
{ title: { _attr: { type: 'html' }, _cdata: entries[i].title }},
{ id: entries[i].link },
{ id: entries[i].id || entries[i].link },
Copy link
Owner

Choose a reason for hiding this comment

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

If you throw an error above at line 109 saying that feed is mandatory, how could link ever be used as an id?

@download13
Copy link
Contributor Author

The feed itself needs an id and each entry also needs an id. That exception is thrown when an id for the feed is left off. When an entry id is left off I just had it keep the default behavior of using the link as the id. It's not ideal but the alternative is throwing for every entry missing an id as well.

@ArtskydJ
Copy link
Contributor

It feels like the id could be abstracted away from the users of this library. Could it default to a uuid so people using this lib don't have to think about it? It would mean another dependency, but a uuid generator is tiny. Here's a uuid generator on npm that I use...

@download13
Copy link
Contributor Author

This would be a hard problem. Ids need to be unique to the entry and never change. These properties are hard to achieve without knowing the context in which someone is using the library.

For example, let's say someone is doing a daily rebuild of their Atom feed for their blog software. They can pass in an id for each article that will always belong to that article. If we generated ids for them, then every time they do a rebuild of the feed the feed id and entry ids will be new random values. Feed readers use the feed id and entry ids to keep track of which feed is which and which entries have been read already. If every entry has a new id every day, feed readers will show every article on the blog as being a new article all the time.

I had looked into using some kind of hash of the title/url/description but that runs into the same problems as just using the title or url as the id. If the entry is modified at all, it's id will change, which is not what we want.

If you can come up with a way to solve this problem that'd be great, but as far as I know the only thing to do is let the end-user handle it.

@ArtskydJ
Copy link
Contributor

Thanks for the explanation! I didn't understand the problem well enough apparently...

I think I'll be using links for now, as I don't plan to change any links on my blog. (To avoid possibly creating dead links.)

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

Successfully merging this pull request may close these issues.

3 participants