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

Add site.feed.updated = latest_post setting #368

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Krinkle
Copy link

@Krinkle Krinkle commented Mar 27, 2022

Two main uses cases:

  1. Reduce deployment churn in repositories that hold a docsite in
    addition to source code. These currently regenerate and find
    something new commit on every change, even when nothing in the
    site has been changed. The only file changing each time is
    feed.xml.
    For example: https://github.com/qunitjs/qunit/commits/gh-pages

  2. Improve reproducibility of the build, as highlighted via
    Respect SOURCE_DATE_EPOCH environment variable (if set) over Time.now jekyll#7187,
    by offering a choice that simply eliminates use of current time entirely,
    not even having to mock it.

Krinkle referenced this pull request in Krinkle/timotijhof.net Mar 27, 2022
@parkr
Copy link
Member

parkr commented Jun 11, 2022

Boolean config options are always restrictive. What if we went with something that allowed the user to decide from one of a list of values? Like feed.updated: latest_post would choose the publish time of the latest post, and feed.updated: site_time (default) would be site.time. Something like that, the values could change but it would grant more freedom in the future.

lib/jekyll-feed/feed.xml Outdated Show resolved Hide resolved
Comment on lines 54 to 55
{% assign last_post = posts | first %}
{% assign last_updated = last_post.last_modified_at | default: last_post.date | default: site.time %}
Copy link
Member

Choose a reason for hiding this comment

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

latest_post.last_modified_at

This is a field from another plug-in, jekyll-last-modified-at, I think. (It requires that Git data is available, which isn't always true.) Looking at dependents, I'm not sure it's entirely worth it to support. Why did you add it here?

Also, if you're not going by published time, then you'd need to iterate through every post to get the latest modification date. Modifying an older post wouldn't reset this time.

Copy link
Author

Choose a reason for hiding this comment

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

I don't mind supporting the jekyll-last-modified-at plugin, as it seems already supported by jekyll-feed. Good point on looping through, I'll amend the change to do that.

I think supporting it is relevant to the <updated> element. This value would otherwise be limited to original publishing dates only. I imagine people who care about this element to use my new option, would likely still want to increment it when they update existing posts, to ensure readers pick it up. Supporting this plugin, seems like a good option to offer for those people, so that at least there is a way to do that.

6000 dependents compared to 1 million for jekyll-feed is small indeed, but 6000 is a lot of sites. A loose search for both suggests there's at least 4000 sites with their source public on github.com using both. I imagine there's more usage among self-hosted sites, on other Git platforms, and in private repos.

Copy link
Contributor

Choose a reason for hiding this comment

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

last_modified_at is already being used by jekyll-feed:

<updated>{{ post.last_modified_at | default: post.date | date_to_xmlschema }}</updated>

It was added in 2015 with a dependency to jekyll-last-modified-at, but this dependency was removed in 2016. Now it relies on manually setting last_modified_at in frontmatter:

https://github.com/jekyll/jekyll-feed/search?q=last_modified_at&type=commits

last_modified_at is also supported by jekyll-seo-tag:

https://github.com/jekyll/jekyll-seo-tag/search?q=last_modified_at&type=code

IMHO jekyll-feed shouldn't stop using it and may use it for additional features.

Primary uses cases:

1. Reduce deployment churn in repositories that hold a docsite in
   addition to source code. These repositories currently regenerate
   and find "changed" files to deploy on every commit, even when
   nothing in the Jekyll site was changed. The only artefact changing
   each time is `feed.xml`.

   For example: https://github.com/qunitjs/qunit/commits/gh-pages

2. Improve reproducibility of the build, as highlighted via
   jekyll/jekyll#7187. By offering a choice
   that simply eliminates use of current time entirely, we do not
   even have to worry about mocking it.
@Krinkle Krinkle changed the title Add feed.updated_deterministic setting Add site.feed.updated = latest_post setting Sep 19, 2022
@parkr
Copy link
Member

parkr commented Mar 3, 2023

Cc @vincerubinetti and #387

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.

None yet

3 participants