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

Added the dbg and format filters #517

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

Conversation

Kixiron
Copy link
Contributor

@Kixiron Kixiron commented May 29, 2020

  • Added the dbg filter which prints the passed value to stdout (similar to the dbg!() macro)
  • Added the format filter, which offers various integer formatting routines such as :x and :X

Closes #474

Copy link
Contributor

@jakeswenson jakeswenson left a 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> {
Copy link
Owner

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

Copy link
Contributor Author

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

Copy link
Owner

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.

Copy link
Contributor

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))
}
Copy link
Owner

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?

Copy link
Contributor Author

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

Copy link
Owner

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

Copy link
Contributor

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.

Copy link
Owner

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.

Copy link
Contributor

@rimutaka rimutaka left a 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> {
Copy link
Contributor

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(':')) {
Copy link
Contributor

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 ?

Copy link
Contributor Author

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))
}
Copy link
Contributor

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.

@Keats
Copy link
Owner

Keats commented Mar 12, 2021

If you can split dbg from format yes

@merkrafter
Copy link

What is your opinion on including the formatting as a parameter to the as_str builtin?
Also, it seems like only formatting a single value is supported, hence using a name (e.g. "hex", "Octal") instead of a format string (like ":x", ":O") might be the easier to understand interface.

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.

Support for formatting
5 participants