-
Notifications
You must be signed in to change notification settings - Fork 285
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
Added the dbg and format filters #517
base: master
Are you sure you want to change the base?
Conversation
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.
Can you add some tests to verify expectations for format
and to prevent regressions in the future?
Other than that looks good!
@@ -145,6 +145,12 @@ pub fn as_str(value: &Value, _: &HashMap<String, Value>) -> Result<Value> { | |||
to_value(&value.render()).map_err(Error::json) | |||
} | |||
|
|||
/// Prints the passed value to stdout before returning it | |||
pub fn dbg(value: &Value, _: &HashMap<String, Value>) -> Result<Value> { |
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 don't think this should be a default filter, people might want to log instead etc
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 don't think it's a bad thing as if someone wants log they can just make one. This is mainly (as with the dbg!()
macro) for debugging. I've found that in nearly every project I've done with Tera I end up recreating this, and it's simple enough to add
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.
It's easy to add as you say so I would keep it out of the builtins one.
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.
Is this function even related to formatting? Should it be a separate PR?
}; | ||
|
||
Ok(Value::String(value)) | ||
} |
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.
Is that exactly what Jinja2 is doing?
Can we add a link to the implementation?
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.
That wasn't based on Jinja, it was just based on Rust, I have no knowledge of what Jinja does
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.
Tera is trying to roughly match what Jinja is doing. The format filter of Jinja is essentially printf (https://jinja.palletsprojects.com/en/2.11.x/templates/#format) so it would need match functionality if it has the same name. We could rename it to avoid confusion
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.
Implementing printf
(https://docs.python.org/3/library/stdtypes.html#printf-style-string-formatting) is quite involved. I wonder how much of would actually get used and if a Rust programmer would know what to do with it. I can see the point of staying close to Jinja2, but matching printf
is more like staying close to Python.
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.
Yep that's fine, just need to rename it so people using both are not confused by having the same name but different functionalities.
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.
@Keats Vincent, I'm happy to put in a few hours into finishing this feature and re-submitting the PR. Can you let me know if we are in agreement re. what needs to be done?
@@ -145,6 +145,12 @@ pub fn as_str(value: &Value, _: &HashMap<String, Value>) -> Result<Value> { | |||
to_value(&value.render()).map_err(Error::json) | |||
} | |||
|
|||
/// Prints the passed value to stdout before returning it | |||
pub fn dbg(value: &Value, _: &HashMap<String, Value>) -> Result<Value> { |
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.
Is this function even related to formatting? Should it be a separate PR?
}; | ||
let mut chars = fmt.chars(); | ||
|
||
if !matches!(chars.next(), Some(':')) { |
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 don't see matches!
macro being used anywhere else in this project. Should it be an IF or MATCH ?
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.
matches!()
is in the stdlib.
}; | ||
|
||
Ok(Value::String(value)) | ||
} |
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.
Implementing printf
(https://docs.python.org/3/library/stdtypes.html#printf-style-string-formatting) is quite involved. I wonder how much of would actually get used and if a Rust programmer would know what to do with it. I can see the point of staying close to Jinja2, but matching printf
is more like staying close to Python.
If you can split dbg from format yes |
What is your opinion on including the formatting as a parameter to the |
dbg
filter which prints the passed value to stdout (similar to thedbg!()
macro)format
filter, which offers various integer formatting routines such as:x
and:X
Closes #474