-
-
Notifications
You must be signed in to change notification settings - Fork 227
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
Publicly explain rationale behind response to #1119 #1128
Comments
Immediately after opening the question, Joshka posted the link into five threads: In this moment I saw this behavior as harassment. I tend to not interact with people I cannot stand, so I closed the issue and hid the following comments. Was this the best way to react? Possibly not, but I won't stand any accusations like "is [that] all you can muster". Be kind. I ask you the same question I asked Joshka, to which they never answered btw: Is anything in frameworks.html / upgrading.html unclear, and how can it be made better? I'd be happy if you could read the documentation and maybe open PRs to enhance it. Please see #1035 (comment) for the reasoning for the upcoming changes. |
Thanks for sharing links to provide more information, more information is often helpful when trying to coordinate groups of people. Just curious if you have a reply to this part of @joshka's most recent comment on that issue:
That seems like a pretty compelling reason to try to figure out a way to make it work, right? I mean, @joshka even volunteered to do the legwork here.
It wasn't until about twenty minutes ago that I found out that Askama is deprecated and Rinja is the new Askama. So perhaps you can forgive me for not looking over there? Is this the section you're referring to?
How can this documentation be made better? By improving the the process. The only thing I think of when reading these docs is "can't this user experience be made better so the docs don't have to be?" |
The intention was not to harass. I can see how this might have come across that way however and for that I apologize. My intent in putting comments on each issue / PR was to close loops to ensure that people watching those issues would be notified that there's some related discussion to parts that were meaningful. It was also to point out the problems concretely in the places where the problems that removing the integration occurred in practice. The purpose of #1119 was to foster some discussion of this which was noticeably absent / not well communicated. Closing it with "Respectfully, no" made me feel incredibly disrespected. There's more discussion on this in the axum discord channel: The need for this feature comes from wanting to be able to test the data that a handler returns rather than the output serialized into a response of the return value. Axum handlers return Documentation is not a good fix for this. The best I can come up with that keeps the things which were possible in Askama 0.12 is to add a wrapper struct and use that everywhere where you would previously just return the template. pub struct HtmlTemplate<T> {
template: T,
}
impl<T: Template> IntoResponse for HtmlTemplate<T> {
fn into_response(self) -> Response {
match self.template.render() {
Ok(html) => Html(html).into_response(),
Err(err) => {
error!("Failed to render template: {err:?}");
StatusCode::INTERNAL_SERVER_ERROR.into_response()
}
}
}
}
impl<T: Template> From<T> for HtmlTemplate<T> {
fn from(template: T) -> Self {
HtmlTemplate { template }
}
}
pub async fn index<T: Todos>(todos: State<T>) -> Result<HtmlTemplate<Index>> {
let todos = todos.all().await?;
Ok(Index::new(todos, Filter::All).into())
}
#[tokio::test]
async fn test_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.template.todos, vec![todo]);
} I suppose it could also be possible to create another derive macro that implements IntoResponse for each template and derive that too. But effectively that was askama_axum. |
I just published askama-derive-axum, which implements an IntoResponse derive macro which replaces the askama_axum crate. use askama::Template;
use askama_derive_axum::IntoResponse;
#[derive(Template, IntoResponse)]
#[template(path = "index.html")]
struct IndexTemplate {
title: String,
body: String,
}
async fn index() -> IndexTemplate {
IndexTemplate {
title: "My Blog".to_string(),
body: "Hello, world!".to_string(),
}
}
The current behavior allowed template rendering to be performed as a crosscutting concern without extra code, and made it possible to unit tests handlers against concrete template types instead of against responses. The documentation links above only show how to workaround the changes for a simplistic use case which doesn't align to how people use Askama currently. The documents could be improved by
|
I guess if I said more clearly (in more than 2 words) why I wasn't happy with the interaction, things would have gone better. Sorry for that! Rather than maintaining n crates, 1 for every framework, I would rather like something similar to https://github.com/Kijewski/rinja_frameworks. I stole your idea to make "IntoResponse" a new derive item. :) That makes it much simpler to not work in lock-step with future askama point releases. Right now, the repo does not have any documentation or ci tests, and only axum and actix-web are implemented. I would invite you, @joshka, to co-maintain the crate if you like to. (Of course, if you would rather keep maintaining your crate, then that's perfectly fine with me, too!) And if @GuillaumeGomez likes the idea to implement the framework integration this way too, I would move it into this org. In any case, I would keep the frameworks section of the book, only mentioning the new crate a possible alternative. Everyone has a different workflow, and I myself would most likely not use the integration crate. In most cases my templates are more likely to contain an |
Thanks, I appreciate your apology. Water under the bridge.
I encourage the stealing here. I'm ambivalent to whether the end result of this ends up as a single or multiple crates (will happily drop the above crate if there's a good alternative that comes out of it).
There's quite a bit more complexity in the implementation than I'd imagined would be necessary (double crate, proc macros wrapping declarative macros), and I haven't fully jumped in to to understand what problems the implementation solves - perhaps you can share some insight into the design? It's highly likely my naive implementation has missed those cases. Happy to work with you on something and deprecate mine when there's a good replacement. I suspect there's some probably some issues with the way I'm importing from the proc macro.
I think it makes sense to provide a few canonical examples here to provide for users who learn top down from examples rather than bottom up from the implementation details. A little documentation now saves a bunch of issues later. |
I released a first version (mostly to secure the name) as |
Cool - I'm going to move continue discussion over in an issue in https://github.com/Kijewski/askama_web/ for better context. Kijewski/askama_web#1 |
Greetings! Longtime Askama user, occasional contributor, confused by all of the recent changes, just trying to get my code working.
I'm trying to sort out what updates need to be made to get various Askama-using projects running on Axum 0.8. I was surprised to find that the askama_axum crate has been deprecated. So I followed the trail and discovered #1119, where @joshka generously put a fair amount of effort into clearly describing the value that such a feature provides.
But @Kijewski can't even be bothered to reply more than two words before closing the issue? I understand you're busy and maintaining open-source projects is a nonstop burden, but seriously:
is all you can muster? Please do your users the benefit of replying in full, or don't bother replying at all. Maybe yet another fork is in order, if this is the approach that current leadership is taking.
The text was updated successfully, but these errors were encountered: