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.

Sign up for GitHub

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

Jump to bottom

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.

@Kijewski Kijewski force-pushed the issue-592 branch 2 times, most recently from db928a9 to 8f0c271 Compare January 5, 2022 17:23
djc
djc reviewed Jan 5, 2022
View reviewed changes
Copy link
Owner

@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
askama/src/lib.rs
@@ -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
Owner

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
askama_actix/Cargo.toml Outdated
@@ -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
Owner

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 force-pushed the issue-592 branch 3 times, most recently from 2007c29 to f75e923 Compare January 6, 2022 14:16
@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.

djc
djc reviewed Jan 6, 2022
View reviewed changes
Copy link
Owner

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

askama_shared/src/input.rs Outdated
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
Owner

@djc djc Jan 6, 2022 •
edited

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 Kijewski force-pushed the issue-592 branch 2 times, most recently from d796abd to 2710792 Compare January 7, 2022 00:20
@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 d796abd before the next big release?

@djc
Copy link
Owner

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 djc:main Jan 7, 2022
@djc
Copy link
Owner

djc commented Jan 7, 2022

Awesome work, thanks!

@Kijewski Kijewski deleted the issue-592 branch January 7, 2022 15:31
@Kijewski Kijewski mentioned this pull request Feb 7, 2022
Merged
@djc djc mentioned this pull request Feb 26, 2023
Closed
@Kijewski Kijewski mentioned this pull request Apr 6, 2024
Merged
Kijewski added a commit to Kijewski/askama that referenced this pull request Apr 6, 2024
@Kijewski
Remove deprecated, no-op features
17a5e00 
The features `"mime"` and `"mime_guess"` are deprecated and do nothing
since [djc#594]. Keeping such deprecated features around can still cause
confusion whether one needs to select them or not. It's been 2,5 year
since djc#594 was merged, so I guess we can drop the features.

[djc#594]: djc#594
@Kijewski Kijewski mentioned this pull request Apr 6, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vallentin vallentin vallentin left review comments

@djc djc djc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Why is there a runtime dependency for mime-guess?
3 participants
@Kijewski @djc @vallentin