search

kaushalmodi / hugo-atom-feed

Hugo theme component for ATOM feed custom Output Format
GNU General Public License v3.0
39 stars 13 forks source link

Enhanced feed attributes and multi-lang support #3

Closed jpawlowski closed 4 years ago

jpawlowski commented 4 years ago

This will add multi-language support for the feed as well as several additional attributes and handlings:

Feed level

Entry level

kaushalmodi commented 4 years ago

It will take me little time to properly review this PR, but looking at the effort you put in writing the change log, it looks super!

It would have been quicker to merge if you retained the indentation offset and left the stylistic changes in followup commits.

I would like to keep the indentation offset to 4 spaces (as that's the norm in Hugo repo), even though I love the 2-space offset which I use in my Nim projects.

kaushalmodi commented 4 years ago

I still haven't thoroughly reviewed this, but based on my initial review, few questions:

use UUID, based on Site.BaseURL instead of site permalink for ID

use UUID, based on relative Permalink instead of site permalink for ID

Why is that necessary? The permalink will still be unique.

add reference to Disqus comment feed if configured

Seems like that change relies on site.Params.comments.engine key to be present.. is that "engine" inside "comments" internal to Hugo?

kaushalmodi commented 4 years ago

@jpawlowski Thanks for your PR.

Please be patient as I manually commit some of the below changes below; will also give reasons why I do I commit those few pieces.

Merged

Merged indirectly

Not merging

jpawlowski commented 4 years ago

You may want to wait a little as I am currently extending the patch.

kaushalmodi commented 4 years ago

@jpawlowski As you see, I see have piecewise commits; one for each feature. Can you create additional PR's rebased off the latest master?

kaushalmodi commented 4 years ago

@jpawlowski

From https://validator.w3.org/feed/docs/atom.html:

id: Identifies the feed using a universally unique and permanent URI. If you have a long-term, renewable lease on your Internet domain name, then you can feel free to use your website's address: http://example.com/

So if UUID is used, it needs to be permanent. In this PR, you are deriving the UUID based off the site.BaseURL. So if the user changes their domain, the UUID will change too.. it's not permanent. So one might as well just use the .Permalink as I do currently. So what I do right now goes along with:

If you have a long-term, renewable lease on your Internet domain name, then you can feel free to use your website's address

The UUID change is adding unnecessary complication without any real gain. What do you think?

jpawlowski commented 4 years ago

@jpawlowski As you see, I see have piecewise commits; one for each feature. Can you create additional PR's rebased off the latest master?

Will probably do that, let me see how to merge this somehow... missing peaces are contributor sections and embedded images. Also, the authors identification is more flexible for multiple themes (as they use different node and formats).

So if UUID is used, it needs to be permanent. In this PR, you are deriving the UUID based off the site.BaseURL. So if the user changes their domain, the UUID will change too.. it's not permanent. So one might as well just use the .Permalink as I do currently. So what I do right now goes along with:

If you have a long-term, renewable lease on your Internet domain name, then you can feel free to use your website's address

The UUID change is adding unnecessary complication without any real gain. What do you think?

I have my own interpretation, based on the IETF RFC document.

  1. Let's separate ID on feed level and entry level.
  2. What happens if you change your blog's domain?
    • Entry IDs shall not change in order to be able to re-identify the same post. Using .RelPermalink would be correct in that case as BaseURL MUST NOT be part of this as otherwise the ID will change if you change your domain.
    • Feed ID shall not change, but it is hard to find a permanent identifier unless the user will explicitly provide one that is independent from the domain.
    • Feed readers will be redirected to the new URL using HTTP 301 redirect which in turn SHOULD make them to change their internal database to the new URL.
    • I doubt that in practice the feed's ID is actually playing an important role in any feed reader (just my guess w/o having proof of it, of course). Even if the ID is playing some role during normal operations, changing the feed URL by being HTTP 301 redirected SHALL actually make the feed reader to re-consider the feed ID at least once, making it the new permanent and unique identifier for normal operation afterwards.

That being said, I believe it is obvious why the entry identifier must be changed to use .RelPermalink. As the RFC says it must be in IRI format, based on RFC3987, we cannot simply use .RefPermalink "as-is". Generating a UUID from .RelPermalink is only one of the valid methods (which in turn is described in RFC2141, as the IRI specification says) which I liked.

Regarding the feed ID, we could actually keep .Permalink but I liked it to be consistent with the entry ID.

kaushalmodi commented 4 years ago

@jpawlowski

get author details from superuser_username of theme provides profile pages for authors and no author details are configured on site level

What is the "super_username" concept? I searched the forums and did not see anyone mentioning it. Seems like not many themes set the "super_username" in the scratch. Even if they do, it looks like a hack.

I'd rather have the users take time to simply set a site.Author.name in their site config to set the author manually if they are so inclined.

jpawlowski commented 4 years ago

What is the "super_username" concept? I searched the forums and did not see anyone mentioning it. Seems like not many themes set the "super_username" in the scratch. Even if they do, it looks like a hack.

I'd rather have the users take time to simply set a site.Author.name in their site config to set the author manually if they are so inclined.

Yea, I agree. It indeed seems a little special to the theme from @gcushen I am using.

kaushalmodi commented 4 years ago

What happens if you change your blog's domain?

I think that's not a frequent occurrence.

Generating a UUID from .RelPermalink is only one of the valid methods

That is also not very stable. I have seen users rename the post URLs and even the subdirectories (post -> posts -> blog -> ..).

So considering that the feed and entry id's are actually a big deal, we can optionally use site.Param.id if available for feed id, and $page.Param "id" if available for entry id.

That way, a user conscious about keeping the id permanent that control it in the params, else it default to the .Permalink. WDYT?

jpawlowski commented 4 years ago

That way, a user conscious about keeping the id permanent that control it in the params, else it default to the .Permalink. WDYT?

That might be the best compromise.

kaushalmodi commented 4 years ago

@jpawlowski

allow to exclude specific pages using .Params.DisableFeed = true

Have you tested this out? It does not work for me.. I have tried using .Params.disablefeed in the template and set disablefeed = true, disablefeed = false and completely removed that param in one of the posts, and still that post always appears in the feed.

Update: That was a user error on my end; I have now merged this bit too, in https://github.com/kaushalmodi/hugo-atom-feed/commit/30770be5f6fb4760b4c7af38cfe14c761bb1de84.

kaushalmodi commented 4 years ago

add summary in addition to content for the feed client to support teaser texts

I had originally added the summary tag (I think) but I had then removed it after reading this:

summary Conveys a short summary, abstract, or excerpt of the entry. Summary should be provided if there either is no content provided for the entry, or that content is not inline (i.e., contains a src attribute), or if the content is encoded in base64. More info here. <summary>Some text.</summary>

from https://validator.w3.org/feed/docs/atom.html.

Given that we are generating feed for blog posts, it will be a very rare occurrence for the .Content to be nil. So I would like to not include summary altogether.

kaushalmodi commented 4 years ago

add page title to title (e.g. for taxonomy nodes)

It's not clear what you mean by that. We already use the .Title if available.

kaushalmodi commented 4 years ago

add reference to Disqus comment feed if configured

add Atom API link to refer to the page comments section

I do not want to hardcode Disqus references in this template. You can have a list of maps that the template can loop through, and from there, you derive the comment service name, link, etc. But more than that, it is not clear what the comment thread has to do with the atom feed.. In the PR, you reference {{ .Permalink }}?utm_source=atom_feed#comments, but what if the user does not have a #comments anchor in the page? There are also references to .Params.commentable, .Params.comments.engine, etc. which look too specific to a theme. May be you can submit this bit as a separate PR and we can discuss more over there.

jpawlowski commented 4 years ago

add page title to title (e.g. for taxonomy nodes)

It's not clear what you mean by that. We already use the .Title if available.

Indeed, we do. What I wanted to say here is that there is still this annoying "on" which does not translate like this into other languages. That's why it was replaced by | but we can also try to fetch a value for it from the users translation tables.

jpawlowski commented 4 years ago

May be you can submit this bit as a separate PR and we can discuss more over there.

This is getting too complicated, I'd rather drop it for now.

kaushalmodi commented 4 years ago

@jpawlowski

Please review the summary in https://github.com/kaushalmodi/hugo-atom-feed/pull/3#issuecomment-518638132.

I have merged almost everything in this commit, except for:

So the only unanswered/unmerged bit is:

add page title to title (e.g. for taxonomy nodes)

If you clarify what that means, I can close this thread cleanly.

jpawlowski commented 4 years ago

@kaushalmodi

Not to mention the reverted patch about summary support :-).

I'll think about it, also considering https://github.com/kaushalmodi/hugo-atom-feed/issues/2.

So the only unanswered/unmerged bit is:

add page title to title (e.g. for taxonomy nodes)

If you clarify what that means, I can close this thread cleanly.

This is already addressed by #7 and essentially was already there ;-) So I guess I may close this pull request, now that I should have found all bugs you introduced by refactoring my code :-p