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

Move all of the template creation into askama_shared #647

Merged
merged 6 commits into from
Mar 23, 2022
Merged

Move all of the template creation into askama_shared #647

merged 6 commits into from
Mar 23, 2022

Conversation

Kijewski
Copy link
Collaborator

Before this PR the handling of integrations was done both by askama_shared and askama_derive. Let askama_shared do the work. This will prevent problems like https://github.com/djc/askama/issues/629, when both packages might come out of sync.

This PR removes the back-and-forth interaction between the two crates. Now askama_derive is a single re-export of #[derive(Template)] which has to be done in a proc_macro crate.

This most likely means that askama_derive is "final", unless another derive template needs to be introduced in the future.

Copy link
Collaborator

@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.

This is looking great, thanks for taking on a more systematic fix for this class of issues!

@@ -0,0 +1,107 @@
extern crate proc_macro;
Copy link
Collaborator

Choose a reason for hiding this comment

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

So I tried this in the past and this doesn't work. As far as I remember, you should not import proc_macro anywhere except in a proc-macro = true crate. However, what we can do is convert to proc_macro2 types in askama_derive and then pass that into askama_shared.

I don't love the derive name of this module, given that I think we might want an attribute macro at some point in the not too distant future, too. Can we use macro as the name, or is that a keyword?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I replaced proc_macro by proc_macro2.

Actually, I guess I could move everything into generator.rs. It's only 100 lines as is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, I guess I could move everything into generator.rs. It's only 100 lines as is.

That probably makes sense for now, although it just means we should start splitting that up since it's already pretty big. Good start, though!

Before this PR the handling of integrations was done both by
askama_shared and askama_derive. This diff lets askama_shared do the
work. This will prevent problems like #629, when both packages might
come out of sync.
All the hard work in askama_derive was actually done in askama_shared.
This PR removes the back-and-forth interaction between the two crates.
Now askama_derive is a single re-export of `#[derive(Template)]` which
has to be done in a proc_macro crate.

This most likely means that askama_derive is "final", unless another
derive template needs to be introduced in the future.
Previously askama_shared exported most of it's internals, so
askama_derive could use them. This is not needed anymore.
@@ -1,6 +1,6 @@
[package]
name = "askama_derive"
version = "0.11.2"
version = "0.12.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this bump?

Copy link
Collaborator Author

@Kijewski Kijewski Mar 23, 2022

Choose a reason for hiding this comment

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

I un-exported a bunch of symbols. Older askama_shared versions would break with this update. Every item in https://github.com/djc/askama/blob/main/askama_derive/src/lib.rs#L8-L10 is gone after the PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed the features section, because they are now handles in askama_shared.

I am not 100% sure, but I guess if I re-added the features, incrementing the patch version only should be fine.

@@ -1,6 +1,6 @@
[package]
name = "askama_shared"
version = "0.12.2"
version = "0.13.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this bump?

Copy link
Collaborator Author

@Kijewski Kijewski Mar 23, 2022

Choose a reason for hiding this comment

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

I un-exported a bunch of symbols. Older askama_derive versions would break with this update. Every item in https://github.com/djc/askama/blob/main/askama_derive/src/lib.rs#L8-L10 is gone after the PR.

@djc djc merged commit b14982f into rinja-rs:main Mar 23, 2022
@Kijewski Kijewski deleted the pr-integrations branch March 23, 2022 18:51
djc pushed a commit that referenced this pull request Mar 30, 2022
The traits Template and DynTemplate need to be in sync with
askama_shared's generator. #647 consolidated the template crating into
askama_shared, this PR moves the trait itself.
@djc djc mentioned this pull request Feb 26, 2023
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.

2 participants