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

Enhanced feed attributes and multi-lang support #3

Closed
wants to merge 6 commits into from
Closed

Enhanced feed attributes and multi-lang support #3

wants to merge 6 commits into from

Conversation

jpawlowski
Copy link
Contributor

@jpawlowski jpawlowski commented Aug 5, 2019

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

Feed level

  • add missing XML declaration as first line
  • use UUID, based on Site.BaseURL instead of site permalink for ID
  • add language to title
  • add page title to title (e.g. for taxonomy nodes)
  • add subtitle (if parameter was configured)
  • add alternate HTML link for other language
  • add alternate links for other output formats besides HTML, including feed in other language
  • add reference to Disqus comment feed if configured
  • add Hugo version and URI to generator
  • get author details from superuser_username of theme provides profile pages for authors and no author details are configured on site level
  • add support for copyright note
  • add icon
  • add logo
  • add limitation support for maximum entries
  • allow to exclude specific pages using .Params.DisableFeed = true

Entry level

  • use UUID, based on relative Permalink instead of site permalink for ID
  • change page parameters for author to authors
  • read author details from author profile page if theme supports it and the author was found
  • add ?utm_source=atom_feed to alternate HTML <link>
  • add translation alternate HTML links
  • add Atom API link to refer to the page comments section
  • add 5 related HTML pages
  • add summary in addition to content for the feed client to support teaser texts

@kaushalmodi
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

kaushalmodi commented Aug 6, 2019

@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

  • add missing XML declaration as first line - 1dd229e
    I saw that your PR used a standalone attribute. I did not know what that was and looked it up, and found https://stackoverflow.com/a/17329699/1219634. Apparently I am not using "DTD", so I feel that is not needed. Please let me know your thoughts in comments.
  • add language to title - 7679fdf
  • add alternate HTML link for other language - 7679fdf
  • add alternate links for other output formats besides HTML, including feed in other language - 7679fdf
  • add Hugo version and URI to generator - 72d0d09
  • add support for copyright note - 51c3a3e
  • add ?utm_source=atom_feed to alternate HTML - 7679fdf
  • add translation alternate HTML links - 7679fdf
  • add limitation support for maximum entries - e46bb23
  • add subtitle (if parameter was configured) - c37a292
  • add 5 related HTML pages - 494b360
  • allow to exclude specific pages using .Params.DisableFeed = true - 30770be

Merged indirectly

  • add icon
    I am not merging this bit as it relies on a hard-coded path img/icon-32.png in site base. Also Hugo does not have an internal site parameter where one can specify the "icon" image path. (Update: added in a different way using Add logo and icon if it was specified by the user #4)
  • add logo
    I am not merging this bit as it relies on a hard-coded path img/icon-512.png in site base. Also Hugo does not have an internal site parameter where one can specify the "logo" image path. (Update: added in a different way using Add logo and icon if it was specified by the user #4)
  • use UUID, based on Site.BaseURL instead of site permalink for ID - see (Update: using id form params in lieu of UUID, merged in Custom identifier for feed and entries #6)
  • use UUID, based on relative Permalink instead of site permalink for ID - see (Update: using id form params in lieu of UUID, merged in Custom identifier for feed and entries #6)
  • add summary in addition to content for the feed client to support teaser texts - see, though now .Summary is supported in a different way (572782c).

Not merging

kaushalmodi added a commit that referenced this pull request Aug 6, 2019
@jpawlowski
Copy link
Contributor Author

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

kaushalmodi added a commit that referenced this pull request Aug 6, 2019
- Thanks to @jpawlowski for adding this in
  #3.

This commit is a modified version of that change where we simply loop
through all .OutputFormats instead of hard-coding each .Get.
@kaushalmodi
Copy link
Owner

@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 added a commit that referenced this pull request Aug 6, 2019
kaushalmodi added a commit that referenced this pull request Aug 6, 2019
- Thanks to @jpawlowski for adding this in
  #3.

This commit is a modified version of how the original author added
this feature in their PR.
@kaushalmodi
Copy link
Owner

@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?

kaushalmodi added a commit that referenced this pull request Aug 6, 2019
@jpawlowski
Copy link
Contributor Author

@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
Copy link
Owner

@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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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 added a commit that referenced this pull request Aug 6, 2019
- Thanks to @jpawlowski for adding this in
  #3.

This is a modified version of the code in that PR.
@kaushalmodi
Copy link
Owner

kaushalmodi commented Aug 6, 2019

@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 30770be.

@kaushalmodi
Copy link
Owner

kaushalmodi commented Aug 6, 2019

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
Copy link
Owner

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 added a commit that referenced this pull request Aug 6, 2019
- Thanks to @jpawlowski for adding this in
  #3.
@kaushalmodi
Copy link
Owner

kaushalmodi commented Aug 6, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 added a commit that referenced this pull request Aug 6, 2019
- Thanks to @jpawlowski for suggesting this in
  #3.

This is a slightly modified version of the code in that PR.
@kaushalmodi
Copy link
Owner

@jpawlowski

Please review the summary in #3 (comment).

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
Copy link
Contributor Author

@kaushalmodi

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

I'll think about it, also considering #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

@jpawlowski jpawlowski closed this Aug 6, 2019
@jpawlowski jpawlowski deleted the patch-1 branch August 6, 2019 22:36
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.

2 participants