Issue 2378 lang level feed config #2400
Conversation
…generate feed for default language
…r default and other languages.
components/config/src/config/mod.rs
Outdated
| .feed_filename | ||
| .as_ref() | ||
| .map(|ff| path.ends_with(ff)) | ||
| .unwrap_or_else(|| path.ends_with(DEFAULT_FEED_FILENAME)) |
There was a problem hiding this comment.
I would maybe put that as a variable above so the condition is easier to read.
There was a problem hiding this comment.
Good call. I have that one changed to keep the if statement more readable.
components/site/src/lib.rs
Outdated
| @@ -1022,6 +1029,19 @@ impl Site { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| pub fn language_feed_filename(&self, lang: &str) -> String { | |||
There was a problem hiding this comment.
Good call. Completely spaced that. Thank you for the heads up.
components/site/src/lib.rs
Outdated
| .config | ||
| .languages | ||
| .get(&self.config.default_language) | ||
| .map_or(false, |lang_opt| lang_opt.generate_feed) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Let me know if the change I have is what you were thinking.
…_filename, and modify config querying for generate_feed value lookup.
|
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 |
| @@ -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. | |||
There was a problem hiding this comment.
need to change the docstring no?
components/config/src/config/mod.rs
Outdated
| @@ -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. | |||
|
Thank you for spotting those docs. They should now be modified to describe how the default values are handled. |
Sanity check:
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 themerge_fieldmacro.generate_feedchecks 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
nextbranch?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.