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
Added support for multiple feeds (i.e. generating both Atom and RSS) #2477
base: next
Are you sure you want to change the base?
Conversation
Ah I just realized I need to run |
Forgot rustfmt 😅 |
I've tested this for my own website and generating two feeds seems to work without a problem, both at the root level of the website and for taxonomies. It should be ready to merge now unless there's some issues I couldn't find. |
@LunarEclipse363 Story of my life. |
For reference, this PR is ready to merge, the force-push was just a rebase where the only conflict was in the changelog. The CI failing now is unrelated to the PR, it looks like some dependency increased its MSRV:
|
Can you rebase on the next branch? I've fixed the CI except for Windows |
components/config/src/config/mod.rs
Outdated
|
||
#[derive(Debug, Clone, Deserialize, PartialEq, Eq)] | ||
#[serde(untagged)] | ||
pub(crate) enum MightBeSingle<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do support that, we need to add a deprecation message when there is only one value. I'm not entirely we need to though, it's a pretty simple change to do from the user POV.
As for name... SingleOrVec
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem I ran into while testing before adding the backwards-compatibility is that Zola doesn't error out if you leave the old value names in the config file, instead using default values, which is a very bad way to handle it. The user will think they have the configuration option set while it's not until they realize it's broken purely due to a single-letter rename.
I can change the name and add a deprecation message, sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: adding a meaningful deprecation message seems to be much harder than I expected due to the constraints of how serde works
@@ -68,8 +68,8 @@ pub struct SectionFrontMatter { | |||
#[serde(skip_serializing)] | |||
pub aliases: Vec<String>, | |||
/// Whether to generate a feed for the current section | |||
#[serde(skip_serializing)] | |||
pub generate_feed: bool, | |||
#[serde(skip_serializing, alias = "generate_feed")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need it here i think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zola will ignore unknown fields here as well though which again ends up with the "suddenly an option stopped working without an error" problem
Which I'm not sure is solvable within serde
}; | ||
let mut feeds = Vec::new(); | ||
for feed_filename in &site.config.feed_filenames { | ||
let mut context = context.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's pretty wasteful.
Can you render them one by one and just updating context.insert("feed_url", &feed_url);
in the loop? I can't remember what additional_context_fn(context);
does though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
render_template()
takes ownership of the context so I need to clone it on every loop regardless
- Fixed language config merge bug found by a test - Adjusted two existing tests to fully check stuff related to multiple feeds - Added a new test for backwards-compatibility of the changes - Fixed bugs found by the newly added test
Introduction
As requested many times before, for example:
Potential improvements
I don't know if/how Zola handles deprecating config options, if there's anything I should add to let the user know the feed related options changed please let me know how to do it.Implemented backwards-compatibility as suggested by @selfisekai
Formalities
Sanity check:
Code changes
next
branch?If the change is a new feature or adding to/changing an existing one: