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

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
Collaborator

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

askama/Cargo.toml Show resolved Hide resolved
@@ -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.

askama/src/lib.rs Show resolved Hide resolved
@@ -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.

askama_actix/src/lib.rs Outdated Show resolved Hide resolved
askama_shared/src/generator.rs Outdated Show resolved Hide resolved
askama_gotham/src/lib.rs Outdated Show resolved Hide resolved
askama_shared/src/input.rs Show resolved Hide resolved
@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
Collaborator 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
Collaborator 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
Collaborator 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 https://github.com/djc/askama/pull/594/commits/d796abdfbb35b9100d2cbced452a4e38109f310a 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 rinja-rs:main Jan 7, 2022
@djc
Copy link
Collaborator

djc commented Jan 7, 2022

Awesome work, thanks!

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.

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