Add possibility to specify template directories #103
Conversation
|
This looks pretty good! I have a bunch of minor concerns, will do a detailed review tomorrow. |
askama/src/lib.rs
Outdated
| @@ -393,7 +393,10 @@ fn visit_dirs(dir: &Path, cb: &Fn(&DirEntry)) -> io::Result<()> { | |||
| /// that have templates, to make sure the crate gets rebuilt when template | |||
| /// source code changes. | |||
| pub fn rerun_if_templates_changed() { | |||
| visit_dirs(&path::template_dir(), &|e: &DirEntry| { | |||
| let trigger_rerun = &|e: &DirEntry| { | |||
There was a problem hiding this comment.
Why pre-define this here? Did you try just passing the closure inside the loop?
askama_shared/Cargo.toml
Outdated
| @@ -10,10 +10,12 @@ workspace = ".." | |||
|
|
|||
| [features] | |||
| default = [] | |||
| serde-json = ["serde", "serde_json"] | |||
| serde-json = [] | |||
There was a problem hiding this comment.
So serde is now required for deserializing the configuration, but I think serde_json should still be optional, as before, no?
askama_shared/src/path.rs
Outdated
|
|
||
| pub fn template_dirs() -> Vec<PathBuf> { | ||
| let root = PathBuf::from(env::var("CARGO_MANIFEST_DIR").unwrap()); | ||
| let askama = root.join("askama.toml"); |
There was a problem hiding this comment.
I would like to move "askama.toml" into a constant, maybe named CONFIG_FILE_NAME. Then, please move the code to instantiate a Config into a Config::from_file() method (which doesn't need any arguments).
Also, here and in other places, please don't override the configuration if it provides an empty list. In that case, it should probably just do nothing (this would allow users to enforce the use of source-based templates only, I guess).
askama_shared/src/path.rs
Outdated
| default | ||
| } | ||
| } else { | ||
| vec![root.join("templates")] |
There was a problem hiding this comment.
This seems to be a duplicate of the default defined earlier.
askama_shared/src/path.rs
Outdated
| path.push(tpl_path); | ||
| let dirs = template_dirs(); | ||
| let path = search_template_in_dirs(&dirs, tpl_path) | ||
| .expect(&format!("Template file '{}' does not exist.", tpl_path.to_str().unwrap())); |
There was a problem hiding this comment.
Nit: elsewhere I use sentence fragments for errors, not full sentences. Please don't capitalize, and don't end with a period.
askama_shared/src/path.rs
Outdated
| @@ -5,9 +5,17 @@ use std::path::{Path, PathBuf}; | |||
|
|
|||
| use toml; | |||
|
|
|||
| fn search_template_in_dirs(dirs: &Vec<PathBuf>, path: &Path) -> Option<PathBuf> { | |||
There was a problem hiding this comment.
Nit: this can take a slice instead of a Vec, so &[PathBuf].
askama_shared/src/path.rs
Outdated
| fs_rel_path.push(rel); | ||
| fs_rel_path = fs_rel_path.with_file_name(path); | ||
| let root = search_template_in_dirs(&dirs, rel).unwrap(); | ||
| let fs_rel_path = root.join(rel).with_file_name(path); |
There was a problem hiding this comment.
One question is whether the start_at paths here are already rooted (that is, are already absolute paths starting with the crate root) or not. Probably need to audit all uses to see whether usage is consistent or not.
There was a problem hiding this comment.
Just preserving existing behavior. If start_at is already rooted the relative path should not exist and it should slip through. This was probably meant for includes where one may not specify the sub-directory the template is in.
There was a problem hiding this comment.
Okay, but can we at least replace the unwrap() with an expect() that clearly states the problem?
askama_shared/src/path.rs
Outdated
| let mut fs_rel_path = root.clone(); | ||
| fs_rel_path.push(rel); | ||
| fs_rel_path = fs_rel_path.with_file_name(path); | ||
| let root = search_template_in_dirs(&dirs, rel).unwrap(); |
There was a problem hiding this comment.
Not sure unwrap() makes sense here?
There was a problem hiding this comment.
I think since start_at is defined this is a path that already existed previously. It just forgot in which directory it existed.
There was a problem hiding this comment.
This should also expect() instead of unwrap().
|
@mashedcode anything else you want to change? Otherwise I'll just merge this and iterate on the design a little bit. Thanks for working on this! |
|
Sorry, wouldn't know how to design it better at the moment. Therefore there's nothing I plan to change at the moment. |
|
If you're interested, here's a bunch of the follow-up I had in mind: https://github.com/djc/askama/compare/b5b1589...f74d121. There's probably a bit more, I wanted to store a |
Resolves #81
I didn't do extensive testing on this because I don't actually use askama in production yet. But it should work. I think.