Merge askama_derive into askama #677

Kijewski · 2022-04-26T09:51:15Z

No description provided.

Kijewski · 2022-04-26T09:54:44Z

Per #676, IMO this makes the project much easier to understand.

djc · 2022-04-26T09:59:03Z

We should keep askama_escape separate, that's actually used by other projects as well. I'll review the rest of this later.

jplatte

So there wasn't really a need for a shared dependency between proc-macro and runtime code at all? 😮

askama_derive/src/input.rs

askama_derive/src/generator.rs

askama/Cargo.toml

Kijewski · 2022-04-26T19:33:49Z

We should keep askama_escape separate, that's actually used by other projects as well. I'll review the rest of this later.

Oh, it's used quite often actually. I did not notice. I unpicked c9e7657.

So there wasn't really a need for a shared dependency between proc-macro and runtime code at all?

Once upon the time maybe there was. :)

This raises the MSRV for 1.60, although it seems like no MSRV than stable is enforced anyways?

I accepted cargo's suggestion to use dep:* without thinking. I use renaming now. The tests now run with 1.53 (any maybe older toolchains, too, but well … 1.53 is the oldest one I have installed except the ancient 1.36 + 1.38). Except for the ui tests (tests that are supposed to fail). Somewhen we broke them for the old versions even though we use version_check.

djc

I think this is a good direction! Don't remember why I didn't do it this way in the first place...

Some nits, I'd like to take a closer look again after this.

djc · 2022-05-18T09:18:48Z

askama/src/lib.rs

@@ -73,7 +73,8 @@ pub use askama_shared::{
 pub mod mime {
    #[cfg(all(feature = "mime_guess", feature = "mime"))]
    #[deprecated(since = "0.11.1", note = "Use Template::MIME_TYPE instead")]
-    pub use crate::shared::extension_to_mime_type;
+    #[allow(non_upper_case_globals)]
+    pub const extension_to_mime_type: () = ();


What's the point of this? It's the same symbol, but it has a different type.

To keep the deprecation notice, so the user gets a message how to proceed even thought their compilation will fail.

It's an interesting idea, but I think we should just get rid of the symbol instead. Probably not a lot of people using it anyway.

askama_derive/src/lib.rs

djc · 2022-05-18T09:24:15Z

askama/Cargo.toml

-askama_shared = { version = "0.13.0", path = "../askama_shared", default-features = false }
+
+comrak = { version = "0.12", optional = true, default-features = false }
+humansize_ = { package = "humansize", version = "1.1.0", optional = true }


Not a fan of the _ suffixes here. Is there another way to do this?

Yes, use a different prefix / suffix 😛

The prefix / suffix doesn't need to be visible in the code, you can rename the dependency back via extern crate humansize_ as humansize; (related post on my blog).

The cleanest solution would be to not rename, and use dep:humansize syntax for the dependency of the feature on the dependency. That is however a new feature from 1.60, so would raise the MSRV.

We can rename the features instead of the dependencies, right?

Sure, that works too. But it often causes a lot of confusion, especially when the name of the optional dependency was previously a feature name. Every optional dependency is automatically a feature (unless shadowed by an explicit feature as of 1.60), so there will be no error for activating the humansize feature.

Kijewski · 2022-05-24T15:12:33Z

askama/src/filters/mod.rs

+use mod_humansize::{file_size_opts, FileSize};
 #[cfg(feature = "num-traits")]
-use num_traits::{cast::NumCast, Signed};
+use mod_num_traits::{cast::NumCast, Signed};


A mod_ prefix is better than a _ suffix, I guess. :) I'm open for other ideas.

There is also the possibility of having overlapping module / external crate names and disambiguating using ::humansize for the crate and self::humansize for the module.

But the problem here isn't module vs crate names but crate name vs feature name, IIRC?

That's what the other review thread is about, this looks separate to me.

No, it's the same issue.

Oh, so mod_ as a crate name prefix? Yeah IMHO that's confusing, and crate would be a better prefix (or suffix).

To me a crate {pre,suf}fix sounds like it's an alias for crate::. How about pkg_*, since the Cargo.toml option is called package?

Well, packages are a little bit of a different concept to crates, but it doesn't seem too bad. Other options could be dep and ext.

Notably dep: is the disambiguation syntax in current versions of Cargo.

Yeah, I guess there's a difference between crates and packages. I'm also sure I will have forgotten about it again tomorrow. :)

I don't like ext_*. So, dep_* it is?

djc · 2022-06-08T08:29:59Z

This has a conflict now, mind rebasing? I'll merge it soon!

Kijewski · 2022-06-08T10:53:49Z

Rebased.

jplatte reviewed Apr 26, 2022

View reviewed changes

askama_derive/src/input.rs Outdated Show resolved Hide resolved

askama_derive/src/generator.rs Outdated Show resolved Hide resolved

askama/Cargo.toml Outdated Show resolved Hide resolved

Kijewski force-pushed the pr-back-into-derive branch from c9e7657 to 44cc276 Compare April 26, 2022 19:26

Kijewski changed the title ~~Remove crates askama_escape and askama_shared~~ Split askama_shared into askama and askama_derive Apr 26, 2022

Kijewski force-pushed the pr-back-into-derive branch 2 times, most recently from 4743fbf to 7713643 Compare April 27, 2022 13:05

djc reviewed May 18, 2022

View reviewed changes

Kijewski mentioned this pull request May 24, 2022

Move code generation into askama_derive #687

Merged

Kijewski force-pushed the pr-back-into-derive branch from 7713643 to fd772cf Compare May 24, 2022 15:10

Kijewski commented May 24, 2022

View reviewed changes

Kijewski changed the title ~~Split askama_shared into askama and askama_derive~~ Merge askama_derive into askama May 24, 2022

Kijewski force-pushed the pr-back-into-derive branch 2 times, most recently from 2b55362 to d7bfeaa Compare May 24, 2022 19:05

Merge askama_derive into askama

9f66bf5

Kijewski force-pushed the pr-back-into-derive branch from d7bfeaa to 9f66bf5 Compare June 8, 2022 10:52

djc approved these changes Jun 8, 2022

View reviewed changes

djc merged commit 6cce13f into djc:main Jun 8, 2022

Kijewski deleted the pr-back-into-derive branch September 21, 2022 11:39

djc mentioned this pull request Feb 26, 2023

0.12 release planning #722

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge askama_derive into askama #677

Merge askama_derive into askama #677

Kijewski commented Apr 26, 2022

Kijewski commented Apr 26, 2022

djc commented Apr 26, 2022

jplatte left a comment

Kijewski commented Apr 26, 2022

djc left a comment

djc May 18, 2022

Kijewski May 18, 2022

djc May 18, 2022

djc May 18, 2022

jplatte May 18, 2022

djc May 18, 2022

jplatte May 18, 2022

Kijewski May 24, 2022

jplatte May 24, 2022

djc May 24, 2022

jplatte May 24, 2022

Kijewski May 24, 2022

jplatte May 24, 2022 •

edited

Kijewski May 24, 2022 •

edited

jplatte May 24, 2022

jplatte May 24, 2022

Kijewski May 24, 2022

djc commented Jun 8, 2022

Kijewski commented Jun 8, 2022

Merge askama_derive into askama #677

Merge askama_derive into askama #677

Conversation

Kijewski commented Apr 26, 2022

Kijewski commented Apr 26, 2022

djc commented Apr 26, 2022

jplatte left a comment

Choose a reason for hiding this comment

Kijewski commented Apr 26, 2022

djc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jplatte May 24, 2022 • edited

Choose a reason for hiding this comment

Kijewski May 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djc commented Jun 8, 2022

Kijewski commented Jun 8, 2022

jplatte May 24, 2022 •

edited

Kijewski May 24, 2022 •

edited