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.

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

RyanAmos
Copy link

  • Tests created for any new feature or regression tests for bugfixes.

Following up on the issue I created here:
#490

This PR introduces two functional changes. The first being supporting a "lax" parsing mode, the second being a "lax" rendering mode. Each additional change extends the functionality, but does not change the default behavior.

Parsing Mode:

In lax parsing mode, an undefined filter will simply return an internal
noop filter. That filter will simply pass along the string to the next
filter, if one is provided.

In strict parsing mode, an undefined filter will continue to return an
error.

Rendering Mode:

In lax rendering mode, an undefined variable will simply be rendered as
a nil value which will ultimately result in an empty string.

In strict rendering mode, an undefined variable will continue to return
an error.

In lax rendering mode, an undefined variable will simply be rendered as
a `nil` value which will ultimately result in an empty string.

In strict rendering mode, an undefined variable will continue to return
an error.
In lax parsing mode, an undefined filter will simply return an internal
noop filter. That filter will simply pass along the string to the next
filter, if one is provided.

In strict parsing mode, an undefined filter will continue to return an
error.
@RyanAmos RyanAmos changed the title Lax rendering and parsing Add optional support for lax rendering and parsing Jan 10, 2023
Comment on lines +6 to +16
#[derive(Clone)]
pub enum ParseMode {
Strict,
Lax,
}

impl Default for ParseMode {
fn default() -> Self {
Self::Strict
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

The 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?

Fair. I can update the name to be 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.

Can you elaborate a bit more what you mean here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate a bit more what you mean here?

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.

Copy link
Author

Choose a reason for hiding this comment

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

Comment on lines +115 to +119
/// Sets the parse mode to lax.
pub fn in_lax_mode(mut self) -> Self {
self.mode = ParseMode::Lax;
self
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd rather re-export the num and accept it.

Copy link
Author

Choose a reason for hiding this comment

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

Comment on lines +6 to +10
#[derive(Clone)]
pub enum ParseMode {
Strict,
Lax,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, no docs on this

Comment on lines +10 to +16
/// What mode to use when rendering.
pub enum RenderingMode {
/// Returns an error when a variable is not defined.
Strict,
/// Replaces missing variables with an empty string.
Lax,
}
Copy link
Member

Choose a reason for hiding this comment

The 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

Comment on lines +6 to +7
#[derive(Clone)]
pub enum ParseMode {
Copy link
Member

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 and Eq

@@ -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 {
Copy link
Member

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 have Copy, Clone, Debug, PartialEq, Eq, and Default

Comment on lines +22 to +30
pub fn render_to(&self, writer: &mut dyn Write, globals: &dyn crate::ObjectView) -> Result<()> {
self.render_to_with_mode(writer, globals, RenderingMode::Strict)
}

/// Renders an instance of the Template, using the given globals in lax mode.
pub fn render_lax(&self, globals: &dyn crate::ObjectView) -> Result<String> {
self.render_with_mode(globals, RenderingMode::Lax)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than having a bunch of different render funtions, should this be stored with the Parser and passed in when this is created?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually originally started that route but decided against it.

When stored on the parser that means you're unable to determine which mode you'd want to use after the parser is created. Every template that's parsed would be stuck in that rendering mode. People would in theory, need to then create two separate parsers to work around that limitation. Admittedly, I'm not sure how frequent people would ever need to change the rendering mode of a parser. I don't personally have a use case to argue that.

Another option is we could expose a function to set the rendering mode on the Template instead. I'm not convinced that's a better UX, but probably a bit less confusing to most users who may use this solely in "strict" mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants