-
Notifications
You must be signed in to change notification settings - Fork 79
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
Add optional support for lax rendering and parsing #492
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,12 +3,25 @@ use super::ParseFilter; | |
use super::ParseTag; | ||
use super::PluginRegistry; | ||
|
||
#[derive(Clone)] | ||
pub enum ParseMode { | ||
Strict, | ||
Lax, | ||
} | ||
Comment on lines
+6
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, no docs on this |
||
|
||
impl Default for ParseMode { | ||
fn default() -> Self { | ||
Self::Strict | ||
} | ||
} | ||
Comment on lines
+6
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a very general name that only covers filters. The name could imply tags, blocks, syntax errors as well. Should we make this more specific? Also, the line for what is parse vs render might be a bit fuzzy for a user that limiting these to just Render and Parse modes obscures the behavior. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fair. I can update the name to be more specific.
Can you elaborate a bit more what you mean here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is it required that filter lookups happen during parse or could it happen during runtime? At the moment, that is an implementation detail that can change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right that the implementation can change, but I personally think the way it's currently implemented is the correct behavior. The way it's currently implemented allows you to store a template that are parsed once and reused multiple times. "Lazily" parsing the template means you delay errors that IMO should be handled when the template itself is created and in turn parsed. Happy to explore alternative options if you think that's the right approach. |
||
|
||
#[derive(Clone, Default)] | ||
#[non_exhaustive] | ||
pub struct Language { | ||
pub blocks: PluginRegistry<Box<dyn ParseBlock>>, | ||
pub tags: PluginRegistry<Box<dyn ParseTag>>, | ||
pub filters: PluginRegistry<Box<dyn ParseFilter>>, | ||
pub mode: ParseMode, | ||
} | ||
|
||
impl Language { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,14 @@ use crate::model::{Object, ObjectView, Scalar, ScalarCow, Value, ValueCow, Value | |
use super::PartialStore; | ||
use super::Renderable; | ||
|
||
/// What mode to use when rendering. | ||
pub enum RenderingMode { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like this should have |
||
/// Returns an error when a variable is not defined. | ||
Strict, | ||
/// Replaces missing variables with an empty string. | ||
Lax, | ||
} | ||
Comment on lines
+10
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to my comments about ParseMode, I feel like this might be too general |
||
|
||
/// State for rendering a template | ||
pub trait Runtime { | ||
/// Partial templates for inclusion. | ||
|
@@ -36,6 +44,9 @@ pub trait Runtime { | |
|
||
/// Unnamed state for plugins during rendering | ||
fn registers(&self) -> &Registers; | ||
|
||
/// Used to set the mode when rendering | ||
fn render_mode(&self) -> &RenderingMode; | ||
} | ||
|
||
impl<'r, R: Runtime + ?Sized> Runtime for &'r R { | ||
|
@@ -78,12 +89,17 @@ impl<'r, R: Runtime + ?Sized> Runtime for &'r R { | |
fn registers(&self) -> &super::Registers { | ||
<R as Runtime>::registers(self) | ||
} | ||
|
||
fn render_mode(&self) -> &RenderingMode { | ||
<R as Runtime>::render_mode(self) | ||
} | ||
} | ||
|
||
/// Create processing runtime for a template. | ||
pub struct RuntimeBuilder<'g, 'p> { | ||
globals: Option<&'g dyn ObjectView>, | ||
partials: Option<&'p dyn PartialStore>, | ||
render_mode: RenderingMode, | ||
} | ||
|
||
impl<'c, 'g: 'c, 'p: 'c> RuntimeBuilder<'g, 'p> { | ||
|
@@ -92,6 +108,7 @@ impl<'c, 'g: 'c, 'p: 'c> RuntimeBuilder<'g, 'p> { | |
Self { | ||
globals: None, | ||
partials: None, | ||
render_mode: RenderingMode::Strict, | ||
} | ||
} | ||
|
||
|
@@ -100,6 +117,7 @@ impl<'c, 'g: 'c, 'p: 'c> RuntimeBuilder<'g, 'p> { | |
RuntimeBuilder { | ||
globals: Some(values), | ||
partials: self.partials, | ||
render_mode: self.render_mode, | ||
} | ||
} | ||
|
||
|
@@ -108,6 +126,16 @@ impl<'c, 'g: 'c, 'p: 'c> RuntimeBuilder<'g, 'p> { | |
RuntimeBuilder { | ||
globals: self.globals, | ||
partials: Some(values), | ||
render_mode: self.render_mode, | ||
} | ||
} | ||
|
||
/// Initialize with the provided rendering mode. | ||
pub fn set_render_mode(self, mode: RenderingMode) -> RuntimeBuilder<'g, 'p> { | ||
RuntimeBuilder { | ||
globals: self.globals, | ||
partials: self.partials, | ||
render_mode: mode, | ||
} | ||
} | ||
|
||
|
@@ -116,6 +144,7 @@ impl<'c, 'g: 'c, 'p: 'c> RuntimeBuilder<'g, 'p> { | |
let partials = self.partials.unwrap_or(&NullPartials); | ||
let runtime = RuntimeCore { | ||
partials, | ||
render_mode: self.render_mode, | ||
..Default::default() | ||
}; | ||
let runtime = super::IndexFrame::new(runtime); | ||
|
@@ -208,6 +237,8 @@ pub struct RuntimeCore<'g> { | |
partials: &'g dyn PartialStore, | ||
|
||
registers: Registers, | ||
|
||
render_mode: RenderingMode, | ||
} | ||
|
||
impl<'g> RuntimeCore<'g> { | ||
|
@@ -268,13 +299,18 @@ impl<'g> Runtime for RuntimeCore<'g> { | |
fn registers(&self) -> &Registers { | ||
&self.registers | ||
} | ||
|
||
fn render_mode(&self) -> &RenderingMode { | ||
&self.render_mode | ||
} | ||
} | ||
|
||
impl<'g> Default for RuntimeCore<'g> { | ||
fn default() -> Self { | ||
Self { | ||
partials: &NullPartials, | ||
registers: Default::default(), | ||
render_mode: RenderingMode::Strict, | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ use std::sync; | |
|
||
use liquid_core::error::{Result, ResultLiquidExt, ResultLiquidReplaceExt}; | ||
use liquid_core::parser; | ||
use liquid_core::parser::ParseMode; | ||
use liquid_core::runtime; | ||
|
||
use super::Template; | ||
|
@@ -19,6 +20,7 @@ pub struct ParserBuilder<P = Partials> | |
where | ||
P: partials::PartialCompiler, | ||
{ | ||
mode: parser::ParseMode, | ||
blocks: parser::PluginRegistry<Box<dyn parser::ParseBlock>>, | ||
tags: parser::PluginRegistry<Box<dyn parser::ParseTag>>, | ||
filters: parser::PluginRegistry<Box<dyn parser::ParseFilter>>, | ||
|
@@ -110,6 +112,12 @@ where | |
.filter(stdlib::Where) | ||
} | ||
|
||
/// Sets the parse mode to lax. | ||
pub fn in_lax_mode(mut self) -> Self { | ||
self.mode = ParseMode::Lax; | ||
self | ||
} | ||
Comment on lines
+115
to
+119
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'd rather re-export the num and accept it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll update, I did it this way since there's only 2 options currently, 1 of which is the default. So exposing a function allowed us to now have to worry about exposing the enum to the user. |
||
|
||
/// Inserts a new custom block into the parser | ||
pub fn block<B: Into<Box<dyn parser::ParseBlock>>>(mut self, block: B) -> Self { | ||
let block = block.into(); | ||
|
@@ -136,12 +144,14 @@ where | |
/// Set which partial-templates will be available. | ||
pub fn partials<N: partials::PartialCompiler>(self, partials: N) -> ParserBuilder<N> { | ||
let Self { | ||
mode, | ||
blocks, | ||
tags, | ||
filters, | ||
partials: _partials, | ||
} = self; | ||
ParserBuilder { | ||
mode, | ||
blocks, | ||
tags, | ||
filters, | ||
|
@@ -152,13 +162,15 @@ where | |
/// Create a parser | ||
pub fn build(self) -> Result<Parser> { | ||
let Self { | ||
mode, | ||
blocks, | ||
tags, | ||
filters, | ||
partials, | ||
} = self; | ||
|
||
let mut options = parser::Language::empty(); | ||
options.mode = mode; | ||
options.blocks = blocks; | ||
options.tags = tags; | ||
options.filters = filters; | ||
|
@@ -178,6 +190,7 @@ where | |
{ | ||
fn default() -> Self { | ||
Self { | ||
mode: Default::default(), | ||
blocks: Default::default(), | ||
tags: Default::default(), | ||
filters: Default::default(), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this should also have
Copy
,Debug
,PartialEq
andEq