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

0.13.0 release planning #1035

Open
7 tasks done
djc opened this issue May 7, 2024 · 19 comments
Open
7 tasks done

0.13.0 release planning #1035

djc opened this issue May 7, 2024 · 19 comments

Comments

@djc
Copy link
Collaborator

djc commented May 7, 2024

We've accumulated quite a bunch of stuff, and should probably release some time soon.

Any thoughts on open issues/PRs (especially changes that affect the public API/behavior) that shouldn't miss the 0.13.0 release?

Here are some open PRs that I think should go in:

@GuillaumeGomez
Copy link
Collaborator

Agreed. It'd be nice to have the book situation fixed too at some point.

@lopld
Copy link

lopld commented May 18, 2024

I am using Askama in my project, and I am very satisfied with it. I have reviewed and used various frameworks, but there was no tool as concise and intuitive as Askama. Please continue to give it your attention. Thank you.

@a-irizi
Copy link

a-irizi commented Sep 1, 2024

hi, i used askama in a rewrite form java spring boot with thymeleaf, and i liked it allot. the only feature that would make it even nicer for me is nested templates blocks. one other thing that i noticed and it would be nice if it got fixed is error reporting, it would be nice if the errors show more context about the template file and the line number as an example.

in general i really enjoyed working with askama, and i'm looking forward for the 0.13 release.

@darrenmothersele
Copy link

It would be nice to get block fragment rendering released. 🙏

@joshka
Copy link

joshka commented Dec 14, 2024

it would be nice if it got fixed is error reporting, it would be nice if the errors show more context about the template file and the line number as an example.

There was a PR or discussion about using Darling a while back. The error message is more descriptive about what the problem is: Ex:

error: Unknown field: `block`
  --> test-darling/src/main.rs:11:29
   |
11 | #[template(path="foo.html", block=bar)]
   |                             ^^^^^

compared with the current error message:

unsupported attribute key Ident { ident: "block", span: #0 bytes(16214..16219) } found

Would this be worth revisiting for better error handling?

@Kijewski
Copy link
Collaborator

There was a PR or discussion about using Darling a while back. The error message is more descriptive about what the problem is: Ex: …

Already fixed in the current rinja version: https://rinja-rs.github.io/play-rinja/?saved=eJwrM4wxUI4uSc0tyEksSdVIrShRsFVQyijJzVHSUUjKyU_OBvKTEos0Y7mKS4pKk0sUQqxjDADnzRF2, which will become the next askama release.

@joshka
Copy link

joshka commented Dec 16, 2024

My point wasn't about the block attribute being unsupported. I'm aware that that is fixed in the unreleased 0.13 version.

My point was that any error message in Askama caused by a misconfiguration leads to an error message which takes longer to diagnose than it should. That can be as simple as a typo in an attribute (blocks vs block), an extra punctuation character (path == "foo.html") or any sort of thing.

@Kijewski
Copy link
Collaborator

I know, and the error message for unknown attributes is good now. Click the link.

@joshka
Copy link

joshka commented Dec 16, 2024

Ah - ok, so there's code which isn't anywhere in this repo which is tested in another repo? Any chance you can expand on what's happening with the reunification with rinja? I don't see any discussion / issue / info about it anywhere.

@Kijewski
Copy link
Collaborator

Yeah, we should really do that. Right now it's handled kinda behind the scenes between @djc, @GuillaumeGomez, and me.

Short answer: We have only a few PRs that need to be fixed, then the current rinja master will become askama 0.13, and the rinja crate will get a message that it's deprecated in favor of askama. But please don't expect any of that to happen in this year, though.

@joshka
Copy link

joshka commented Dec 16, 2024

Sounds like a good idea that would be positive for the project's health. Looking forward to it :)

@a-irizi
Copy link

a-irizi commented Dec 25, 2024

Would this be worth revisiting for better error handling?

certainty. but the current message is fine too so i wouldn't give it a high priority if it's allot of work to get it done.

the biggest issue that i got was when compiling code and my template had an error, when compiling i got a hint of what's wrong, along with rust code (i guess the code generated from the templates).

what i rather have is a more user friendly message that shows me where in my template (HTML not rust) was there an error (something like miette as an example)

this happened allot when i made a some fields optional. and now instead of having an error message showing me where exactly in my HTML template did i mess up. it shows me some rust code that i have to decipher.

this was the biggest pain point for me aside from not being able to use nested blocks.

otherwise i think this is a very solid project, one that relieves you from the mental overhead of always checking that a DTO and a template are always in sync and only finding out that they are not at runtime (which is why i enjoyed it allot but i came across the debugging issue).

if it's not clear i can provide some code examples if you want

@rfdonnelly
Copy link

Here's another for 0.13. It fixes askama for the recently released axum 0.8.

@GuillaumeGomez
Copy link
Collaborator

We dropped askama_axum so no need to fix it. We instead added a chapter on how to convert Result. Example here.

@joshka
Copy link

joshka commented Jan 12, 2025

We dropped askama_axum so no need to fix it. We instead added a chapter on how to convert Result. Example here.

@joshka
Copy link

joshka commented Jan 12, 2025

#1119 was closed by @Kijewski with the message "Respectfully, no" and the comments on the places that call out the problems with the approach to removing the integration pieces were closed as off topic (they were literally on the exact topic). To close off an issue that describes a problem like that is anything but respectful. I'd like to ask that we engage in some discussion around this to solve the issue of integration with axum for apps. I also share the same opinion that the existing code was sub-optimal and would like to work out something that is more appropriate rather than a full removal of that code. Are you open to discussing this more, or does this project not welcome outside contribution and input of its users anymore?

When Askama is used in a web app, converting the template into an response that's sent to the user is a cross cutting concern. One that has to be done on every use of the template. Implementing IntoResponse seems like the obvious approach for this. But due to the orphan rule, the only place where that can reasonably be done is within the macro.

The benefit to this approach that you can write your handlers as accepting the input they require and outputting a template, which makes them super easy to unit test. You don't have to test the html, you instead test with just data.

Pinging @djc and @GuillaumeGomez for their input on this.

@GuillaumeGomez
Copy link
Collaborator

I didn't say anything because I agreed with @Kijewski: it's something we discussed and considering the small amount of code necessary to achieve this result, we decided to instead explain in the book how to do it (done in this PR) rather than having to maintain these integration crates.

Are you open to discussing this more, or does this project not welcome outside contribution and input of its users anymore?

I know you speak out of frustration but please remember we're all humans here (I hope) and we have reasons. Let me explain them more in details and see if it helps you understand why we decided to go this way and how we think it's, if not better, at least not worst than the current situation:

  • We'd need to follow closely these frameworks updates and make releases when they update. Which brings an issue: the versioning might not match with the main crate anymore. It's secondary but still something we considered was worth taking into account.
  • We don't consider it worth it to keep maintaining crates with such little code. We think that showing to users how they can do it and letting them copy-paste the code if they only need the basic one is more than enough.

If the examples/docs are not good enough, please tell us what's blocking you so we can improve them and ensure others in your situation will not be blocked.

@joshka
Copy link

joshka commented Jan 12, 2025

Thank you for your response

I know you speak out of frustration but please remember we're all humans here (I hope) and we have reasons. Let me explain them more in details and see if it helps you understand why we decided to go this way and how we think it's, if not better, at least not worst than the current situation:

My frustration on this stems from the closing of the issue without discussion other than "Respectfully, no". As a fellow human I hope you can understand how that is a frustrating way to engage in dialog. I thought that maybe creating an issue to start the discussion and keep it self-contained there would be the right thing to do. Thank you for being open to restarting that dialog rather than marking my comment above as off-topic / closed. I'm happy to discuss this however you'd please to work out a better way forward. Would re-opening #1119 make sense rather than shoehorning the discussion into this issue?

We'd need to follow closely these frameworks updates and make releases when they update. Which brings an issue: the versioning might not match with the main crate anymore. It's secondary but still something we considered was worth taking into account.

This is definitely a reasonable pain point, but it has a fairly reasonable solution which is regularly used across the rust ecosystem. Feature flag on specific versions. Specifically that approach is to:

  1. rely on the core crate of a library, which generally is more stable and doesn't change greatly while the main crate can (e.g. axum-core)
  2. feature flag on more than one version of the crate (e.g. axum-core-0_4, axum-core-0_5). This allows apps to upgrade when they upgrade their own dependency to match the choice of web lib.

We don't consider it worth it to keep maintaining crates with such little code. We think that showing to users how they can do it and letting them copy-paste the code if they only need the basic one is more than enough.

On a github dependents search, there are 10786 repos that use askama-derive, 965 usages of askama-axum, 235 for askama-actix, 66 for rocket, 54 for tide and 78 for warp (total 1,398 or 13% of the askama-derive usages). And the download counts are similar. For each of these projects every single usage of the template will now have to have extra code to convert from the template into a response for the specific framework. This code is non-differentiated and has to be added to every handler in the project if it is not built in to Askama.

It's not that the amount of code is difficult to add that is the problem here, it's that that code has to be added everywhere. I don't think that the code necessarily belongs it its own crate like the current situation leads to, perhaps more usefully just belonging in the derive / main crate under a feature flag would be fine.

I wonder whether you could expand on the MIME removal parts of the problem. Was that a relevant part of the decision to remove this functionality? Thinking about this from an greenfield implementation perspective. I'd want the following code to do the right thing.

#[derive(Template)]
#[template(path = "index.html")]
pub struct Index {
    pub todos: Vec<Todo>,
}

pub async fn index<T: Todos>(todos: State<T>) -> Result<Index> {
    let todos = todos.all().await?;
    Ok(Index { todos })
}

Where the right thing is effectively:

impl IntoResponse for Index {
    fn into_response(self) -> Response {
        let body = self.render().unwrap();
        Html(body).into_response()
    }
}

This allows for writing the following unit tests of the logic of the handlers without having to string compare the template response:

#[tokio::test]
async fn index() {
    let todo = db::Todo {
        id: 1,
        text: "hello".to_string(),
        completed: false,
    };
    let todos = vec![todo.clone()];
    let mut mock = MockTodos::new();
    mock.expect_all().once().return_once(|| Ok(todos));

    let response = super::index(State(mock)).await.unwrap();

    assert_eq!(response.todos, vec![todo]);
}

But then I find myself concerned with what about json / text / other types that are templated (svg, ...). Obviously the Mime guessing was there for a reason. A suggestion on the Mime issue was to make the mime type part more explicit, with a field to control the type. Maybe that's worth exploring / discussing.

Also, I want to make it clear that I'm not asking for you or the other contributors to put the time in for particular implementations, I'm happy to put forward PRs and do the work to make this happen, but to do that requires being on the same page with them about how the direction of the project will be moving.

@GuillaumeGomez
Copy link
Collaborator

My frustration on this stems from the closing of the issue without discussion other than "Respectfully, no". As a fellow human I hope you can understand how that is a frustrating way to engage in dialog. I thought that maybe creating an issue to start the discussion and keep it self-contained there would be the right thing to do. Thank you for being open to restarting that dialog rather than marking my comment above as off-topic / closed. I'm happy to discuss this however you'd please to work out a better way forward. Would re-opening #1119 make sense rather than shoehorning the discussion into this issue?

For you as a newcomer, it is a bit of an extreme reaction indeed. For us who discussed about it a lot (and had a lot of issues opened about it as well), it's just yet another issue about it. The discussion is already over. However, how we could make this transition easier for our users is still an open discussion.

This is definitely a reasonable pain point, but it has a fairly reasonable solution which is regularly used across the rust ecosystem. Feature flag on specific versions. Specifically that approach is to:

1. rely on the core crate of a library, which generally is more stable and doesn't change greatly while the main crate can (e.g. axum-core)

2. feature flag on more than one version of the crate (e.g. axum-core-0_4, axum-core-0_5). This allows apps to upgrade when they upgrade their own dependency to match the choice of web lib.

That's still more work than what we think is worth.

...

For the MIME type deletion, instead of trying to guess when to escape or not, we decided to instead add a Safe type which will indicate what content needs to be escaped or not. The discussion about it is here and the implementation is here.

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

No branches or pull requests

8 participants