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

Determine Content-Type during compilation #594

Merged
merged 5 commits into from
Jan 7, 2022
Merged

Determine Content-Type during compilation #594

merged 5 commits into from
Jan 7, 2022

Conversation

Kijewski
Copy link
Member

@Kijewski Kijewski commented Jan 5, 2022

Closes #592.

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.

I think the core idea here is great, looks like you brought a bunch of other changes along that I think need a separate discussion.

@@ -91,6 +91,9 @@ pub trait Template {

/// Provides a conservative estimate of the expanded length of the rendered template
const SIZE_HINT: usize;

/// The MIME type (Content-Type) of the data that gets rendered by this Template
const MIME_TYPE: &'static str;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we deprecate EXTENSION and extension()? I feel like this is probably better.

@@ -14,7 +14,7 @@ edition = "2018"

[dependencies]
actix-web = { version = "4.0.0-beta.19", default-features = false }
askama = { version = "0.11.0", path = "../askama", default-features = false, features = ["with-actix-web", "mime", "mime_guess"] }
askama = { version = "0.11.0", path = "../askama", default-features = false, features = ["with-actix-web"] }
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 guess semver-compat means we should keep it here, as well.

@djc djc marked this pull request as draft January 5, 2022 17:55
@Kijewski Kijewski marked this pull request as ready for review January 6, 2022 14:25
@Kijewski Kijewski changed the title WIP: determine Content-Type during compilation Determine Content-Type during compilation Jan 6, 2022
@Kijewski
Copy link
Member Author

Kijewski commented Jan 6, 2022

I refactored the PR.

The last commit "Remove ext argument in integrations" is probably a breaking change according to semver. I'm not sure if the functions in the integration crates (e.g. askama_axum::into_response) can be considered to be part of the public interface, they have no documentation after all.

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.

Sorry for the somewhat iterative review, but got some more changes I'd like to the PR.

Also I think we should ditch the last commit that removes the _ext arguments in the integrations to preserve compatibility, even if it's undocumented API.

Comment on lines 184 to 197
const TEXT_TYPES: [(mime::Mime, mime::Mime); 6] = [
(mime::TEXT_PLAIN, mime::TEXT_PLAIN_UTF_8),
(mime::TEXT_HTML, mime::TEXT_HTML_UTF_8),
(mime::TEXT_CSS, mime::TEXT_CSS_UTF_8),
(mime::TEXT_CSV, mime::TEXT_CSV_UTF_8),
(
mime::TEXT_TAB_SEPARATED_VALUES,
mime::TEXT_TAB_SEPARATED_VALUES_UTF_8,
),
(
mime::APPLICATION_JAVASCRIPT,
mime::APPLICATION_JAVASCRIPT_UTF_8,
),
];
Copy link
Collaborator

@djc djc Jan 6, 2022

Choose a reason for hiding this comment

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

I'd prefer to outline the const here, just dump it near the bottom of the module (or in the crate root?). Also, could there be a separate commit that moves the logic from askama::mime into askama_shared, forwarding the methods from the old place such that we don't duplicate the logic, and also a separate commit that changes the logic to what you have here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@Kijewski
Copy link
Member Author

Kijewski commented Jan 7, 2022

Sorry for the somewhat iterative review, but got some more changes I'd like to the PR.

No problem!

Also I think we should ditch the last commit that removes the _ext arguments in the integrations to preserve compatibility, even if it's undocumented API.

I omitted the diff to remove the "ext" parameters. Maybe we can cherry-pick d796abd before the next big release?

@djc
Copy link
Collaborator

djc commented Jan 7, 2022

I omitted the diff to remove the "ext" parameters. Maybe we can cherry-pick d796abd before the next big release?

Sounds good. Or even file it as a draft PR including something about compatibility in the title?

@djc djc merged commit 332d741 into askama-rs:main Jan 7, 2022
@djc
Copy link
Collaborator

djc commented Jan 7, 2022

Awesome work, thanks!

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.

Why is there a runtime dependency for mime-guess?
3 participants