Skip to content
This repository was archived by the owner on Mar 11, 2025. It is now read-only.

feature: Add embed node #888

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

YannikSc
Copy link

@YannikSc YannikSc commented Oct 24, 2023

In memory of my old issue/feature request in #488, here is finally the implementation of it. Tests are included of cause.

I have (not yet) updated the documentation for it. If this feature and implementation is fine, I'm open for updating the documentation to describe the new possibilities.

Edit: I did update the documentation now, as I think it might be easier to understand, what it's actually doing.

Copy link
Contributor

@wrapperup wrapperup left a comment

Choose a reason for hiding this comment

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

This is a really cool (and useful!) feature, it's sort of like components in a way. Had a few thoughts and nits.

.config
.find_template(embed.path, Some(&self.input.path))?;
let mut embedded_context =
Context::new(self.input.config, &self.input.path, embed.nodes.as_slice())?;
Copy link
Contributor

@wrapperup wrapperup Nov 7, 2023

Choose a reason for hiding this comment

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

There's missing errors for doing anything outside block tags. Some of these don't actually do anything (extends for example gets overwritten), but you're allowed to write macro and include nodes here, which seems weird (this is similar behavior to how normal extended templates work, so maybe not a real issue?)

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I see why this is problematic, but it's consistent. I think this is something, that should be fixed/improved in general.

Another real problem, that I mentioned in the issue (#488) is, that the compiler is not including the file when the embed is inside a block in an extended file. This problem however also exists for includes and I'm not sure how to approach this.

@Kijewski Kijewski force-pushed the main branch 2 times, most recently from 5633250 to 704f8f1 Compare March 11, 2025 19:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants