Support for loading templates from the file system or from an embedded representation in the app's binary. #171
Conversation
… system or from the binary of the app.
| @@ -10,7 +10,6 @@ COPY crates /build/crates | |||
| COPY data /build/data | |||
| COPY src /build/src | |||
| COPY tests build/tests | |||
| COPY diagnostic_templates /build/diagnostic_templates | |||
There was a problem hiding this comment.
No longer needed as the default templates are now embedded into the app's binary.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #171 +/- ##
=======================================
+ Coverage 75.0% 75.5% +0.4%
=======================================
Files 43 44 +1
Lines 2729 2849 +120
=======================================
+ Hits 2049 2153 +104
- Misses 680 696 +16 ☔ View full report in Codecov by Sentry. |
crates/weaver_forge/src/lib.rs
Outdated
|
|
||
| /// The file loader function with a 'static lifetime (required by MiniJinja). | ||
| loader_function: | ||
| Arc<dyn for<'a> Fn(&'a str) -> Result<Option<String>, Error> + Send + Sync + 'static>, |
There was a problem hiding this comment.
Used Arc because templates are loaded in parallel.
There was a problem hiding this comment.
I get that, but the design feels awkward to me.
- This is exposing A LOT of details publicly - Arc, Error, Send/Sync, etc.
- I'd prefer keeping raw types e.g. (T) with a trait you can use.
For example, instead of returning a loader-function, why not just expose the function itself and keep the loader in an Arc if you want? Then the owner of the Loader is responsible for keeping it in an arc/safely and generates their own thread-safe closure.
pub trait FileLoader {
/// Returns a textual representation of the root path of the loader.
/// This representation is mostly used for debugging and logging purposes.
fn root(&self) -> &Path;
/// Returns a list of all files in the loader's root directory.
fn all_files(&self) -> Vec<PathBuf>;
/// Loads a file from a given name
fn load_file<'a>(
&self,
&'a str
) -> Result<Option<String>, Error>;
}
(caveat you may need to sprinkle some Send+Sync in there)
Then, in weaver_forge you can have an Arc<dyn FileLoader> if you need it, or you can otherwise synchronize threading access to the trait as you want.
TL;DR; If this is a public trait, I think its impl is way too tiightly coupled to how we're doing threading today with little flexibility.
There was a problem hiding this comment.
I agree, the interface is too complex. I'm going to simplify this as much as possible.
lquerel
changed the title
| use crate::error::Error::TargetNotSupported; | ||
|
|
||
| /// An abstraction for loading files from a file system, embedded directory, etc. | ||
| pub trait FileLoader: Send + Sync { |
There was a problem hiding this comment.
Nit: Can you add the Send + Sync requirement only in the weaver_forge where you need the type to be send+sync?
crates/weaver_forge/src/lib.rs
Outdated
| /// Template path | ||
| path: PathBuf, | ||
| /// File loader used by the engine. | ||
| file_loader: Arc<dyn FileLoader + 'static>, |
There was a problem hiding this comment.
nit: Here can you add the +Send + Sync?
|
Looks like the docker build also needs tobe updated for the rename of the templates. |
…he FileLoader trait.
This PR contains the following changes:
weaver diagnostic initto locally create diagnostic templates, allowing the user to override them or define new targets.Closing: #167