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

Issue 2378 lang level feed config #2400

Open
wants to merge 10 commits into
base: next
Choose a base branch
from

Conversation

SumDonkuS
Copy link
Contributor

@SumDonkuS SumDonkuS commented Jan 4, 2024

Sanity check:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Code changes

Looking at issue 2378 and the related issue 2360, found that there were a few places that needed to check the LangaugeOptions. To handle the change for detecting the settings for feed_filename, the code has been switched to having this config be optional. This also allowed for switching back to the first implementation of the merge_field macro. generate_feed checks both the section for the default language as well as the global area for its config. If it exists in both places an error is thrown for the collision.

  • Are you doing the PR on the next branch?

  • Have you created/updated the relevant documentation page(s)?

No documentation change for this bug fix.

This is my first PR for this project. Please let me know if there is anything that I'm missing. Thank you again for the great project.

.feed_filename
.as_ref()
.map(|ff| path.ends_with(ff))
.unwrap_or_else(|| path.ends_with(DEFAULT_FEED_FILENAME))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would maybe put that as a variable above so the condition is easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I have that one changed to keep the if statement more readable.

@@ -1022,6 +1029,19 @@ impl Site {
Ok(())
}

pub fn language_feed_filename(&self, lang: &str) -> String {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we not return &str?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Completely spaced that. Thank you for the heads up.

.config
.languages
.get(&self.config.default_language)
.map_or(false, |lang_opt| lang_opt.generate_feed)
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like we do need better config querying based on a language like you did with language_feed_filename.
No need to change anything here, just thinking aloud

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if the change I have is what you were thinking.

@SumDonkuS
Copy link
Contributor Author

Thank you for catching those things for me. I'll keep a closer eye out for those in the future. Let me know if the modification to querying generate_feed from the config looks like what you were thinking. Thank you again.

@SumDonkuS SumDonkuS requested a review from Keats January 14, 2024 21:16
Copy link
Collaborator

@Keats Keats left a comment

Choose a reason for hiding this comment

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

I think it's ok. A bit iffy with Option but maybe it's fine

@@ -18,7 +18,7 @@ pub struct LanguageOptions {
pub generate_feed: bool,
/// The filename to use for feeds. Used to find the template, too.
/// Defaults to "atom.xml", with "rss.xml" also having a template provided out of the box.
Copy link
Collaborator

Choose a reason for hiding this comment

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

need to change the docstring no?

@@ -66,7 +67,7 @@ pub struct Config {
pub feed_limit: Option<usize>,
/// The filename to use for feeds. Used to find the template, too.
/// Defaults to "atom.xml", with "rss.xml" also having a template provided out of the box.
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

@SumDonkuS
Copy link
Contributor Author

Thank you for spotting those docs. They should now be modified to describe how the default values are handled.

@SumDonkuS SumDonkuS requested a review from Keats January 27, 2024 17:43
@Keats
Copy link
Collaborator

Keats commented May 27, 2024

Note: that PR conflicts with #2477, i'll have a look at rebasing it after 2477 is merged

@Keats
Copy link
Collaborator

Keats commented Jun 19, 2024

The other PR has been merged, do you have time to rebase it or should i do it?

@Keats
Copy link
Collaborator

Keats commented Jun 20, 2024

There seems to be more spots to fix than I thought. I'll release the next version of Zola first and get back to that after, otherwise I'm never going to do that release.

@SumDonkuS
Copy link
Contributor Author

Sorry for the delay. Work pulled me away for a bit.

* Refine YAML date regex

This commit does a few changes:

- Introduce a new regex
  - it is a bit off-spec (it allows one-digit months and days in date-only mode)
  - uses named groups
  - avoids group duplication
- parses offset once

Fixes getzola#2538

* Fix nanosecond parsing

* Rename variables for brewity

* Add tests
@Keats
Copy link
Collaborator

Keats commented Jul 1, 2024

I've put a tiny commit on the next branch: 4c5cc53 for some obvious issues.
Do you want to restart work on that PR? Probably better to start from scratch tbh

@SumDonkuS SumDonkuS force-pushed the issue-2378-lang-level-feed-config branch from 0a65981 to 8d79d25 Compare July 2, 2024 01:44
@SumDonkuS SumDonkuS force-pushed the issue-2378-lang-level-feed-config branch from f3c2fdd to 71da1d5 Compare July 2, 2024 02:06
@SumDonkuS
Copy link
Contributor Author

Sorry. Missed your comment about starting fresh after rebasing. Let me know if you would like to start from scratch.

self.render_feeds(pages, Some(&PathBuf::from(code)), code, |c| c)?;
start = log_time(start, "Generated feed in other language");
}
self.render_all_feeds()?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This name is not accurate since there are also section feeds. render_site_feeds?

let is_default_language = code == &self.config.default_language;

let skip_default_language_feed_generation =
is_default_language && !self.config.generate_feeds && !language.generate_feeds;
Copy link
Collaborator

Choose a reason for hiding this comment

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

should that ambiguity be resolved when loading the config? Eg error if the value at the top level and in a default language level differs.
This way you can now just do something like self.config.languages.iter().filter(|x| x.generate_feeds) for the loop

@@ -1209,3 +1226,200 @@ fn log_time(start: Instant, message: &str) -> Instant {
}
now
}

#[cfg(test)]
mod tests {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can those be moved in the tests folders? This file is already long enough without tests

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