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

Feature: Block fragments #824

Merged
merged 1 commit into from Apr 25, 2024
Merged

Feature: Block fragments #824

merged 1 commit into from Apr 25, 2024

Conversation

wrapperup
Copy link
Contributor

@wrapperup wrapperup commented Jun 18, 2023

Since #568 hasn't had an update in a long time, and it is a nice feature, this is a new PR to implement the proposed changes from that discussion.

This adds block fragments/partials as part of the Template derive. Here's an example of how to use it:

#[derive(Template)]
#[template(path = "...", block = "my_block")]
struct MyBlockFragment<'a> {
    name: &'a str,
}

This also properly supports inheritance, nesting, and the super() macro. It does this by abstracting buf_writable in the generator to add the ability to discard output, except for the block that needs to be rendered.

Copy link
Contributor

@couchand couchand left a comment

Choose a reason for hiding this comment

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

I'm really impressed at how effective the strategy of abstracting the WritableBuffer is. Very nice.

Personally this feature seems a bit odd to me. It feels like its overloading the block inheritance mechanism to do something very different that there's already other ways to do. I did re-read the thread from #568. I wonder if perhaps a more complete motivating example would be helpful, in the tests or the docs or just PR comments?

askama_derive/src/generator.rs Outdated Show resolved Hide resolved
book/src/creating_templates.md Outdated Show resolved Hide resolved
book/src/template_syntax.md Outdated Show resolved Hide resolved
@wrapperup
Copy link
Contributor Author

wrapperup commented Jun 24, 2023

Personally this feature seems a bit odd to me. It feels like its overloading the block inheritance mechanism to do something very different that there's already other ways to do.

I couldn't think of a cleaner way here. Perhaps adding another node to do it, but block ended up being suggested as the preferred way in that thread.

I wonder if perhaps a more complete motivating example would be helpful, in the tests or the docs or just PR comments?

It's very much a convenience feature. There are already ways to do achieve the same result (macros, importing other templates). When using a library like htmx. It's pretty nice to just go slap in a couple of lines in a template file to say "hey, this something that will update", and it can.

It's part of this whole "locality of behavior" idea, where you keep things as close together as possible (ideally, in the same file). htmx, Alpine.js, Tailwindcss, and all of those that are HTML first trend towards this kind of design. This is also a pretty good read: https://htmx.org/essays/template-fragments/

Copy link
Owner

@djc djc left a comment

Choose a reason for hiding this comment

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

The direction here seems like it offers a decent amount of functionality especially relative to the limited complexity cost, so it seems reasonable to me. @vallentin / @Kijewski any thoughts?

testing/tests/block_fragments.rs Show resolved Hide resolved
askama_derive/src/generator.rs Outdated Show resolved Hide resolved
askama_derive/src/generator.rs Outdated Show resolved Hide resolved
@wrapperup
Copy link
Contributor Author

wrapperup commented Aug 11, 2023

The parser crate changes had no effect on this PR, which is great. Could I get eyes on these changes again just incase?

@wsantos
Copy link

wsantos commented Aug 23, 2023

@djc what do we need to merge this? I'm considering using it but since my site is going to be built in htmx this is a requirement, thanks in advance.

@djc
Copy link
Owner

djc commented Aug 24, 2023

I'd like a second opinion from one of the other maintainers.

@nathanaelhuffman
Copy link

I'm also considering using this with htmx and would love to see this land.

@pbouzakis
Copy link

Same here; looking to leverage this in htmx project. Currently, I'm using minijinja and render_block, which is working nicely. Would love to move to Askama if I can, but need this feature.

@teenjuna
Copy link

teenjuna commented Oct 7, 2023

Hey @Kijewski, sorry to bother, but it looks like @djc is looking for your opinion on this. Can you please take a look? 🙏

@djc
Copy link
Owner

djc commented Oct 9, 2023

(No is also an okay answer, but would like to have that be made explicit!)

@Kijewski
Copy link
Collaborator

Kijewski commented Oct 9, 2023

Sorry, I won't find the time to wrap my head around this PR for the foreseeable time. At a very (very) quick glance the code looks fine, and the feature seems useful to me.

@James-Rhodes
Copy link

Hi all is there any updates on this feature? Very much looking forward to seeing it land :)

@djc
Copy link
Owner

djc commented Oct 25, 2023

This is waiting for another round of review from me now that we have some positive acknowledgement from @Kijewski (thanks!) -- I'm having a very busy time so it may take me a bit.

(If this is important to you, maybe consider sponsoring my work on Askama and other projects.)

@GuillaumeGomez
Copy link
Collaborator

The feature is pretty nice and code looks good to me! Let's wait for @djc to have time to come back to it.

@wrapperup wrapperup requested a review from djc March 26, 2024 05:37
book/src/creating_templates.md Outdated Show resolved Hide resolved
book/src/template_syntax.md Outdated Show resolved Hide resolved
book/src/template_syntax.md Outdated Show resolved Hide resolved
askama_derive/src/input.rs Outdated Show resolved Hide resolved
askama_derive/src/lib.rs Outdated Show resolved Hide resolved
@djc
Copy link
Owner

djc commented Apr 7, 2024

Please just rebase and squash all your changes -- right now "this branch cannot be rebased due to conflicts".

@wrapperup wrapperup force-pushed the block-fragments branch 2 times, most recently from fc0c328 to 179434c Compare April 14, 2024 01:45
@djc
Copy link
Owner

djc commented Apr 15, 2024

(Failing CI now.)

@Kijewski
Copy link
Collaborator

Kijewski commented Apr 18, 2024

I think the error is unrelated to this PR, and it should be fixed in the current main branch. Can you please rebase, @wrapperup?

@wrapperup
Copy link
Contributor Author

I think the error is unrelated to this PR, and it should be fixed in the current main branch. Can you please rebase, @wrapperup?

Yep! Was just waiting on it to be fixed in main.

@djc djc merged commit 1347d36 into djc:main Apr 25, 2024
17 checks passed
@djc
Copy link
Owner

djc commented Apr 25, 2024

Thanks, and sorry for the long delays in getting this merged!

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

10 participants