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 more template functions according to fermyon/bartholomew#26 fermy… #4

Merged
merged 1 commit into from
Sep 16, 2022

Conversation

karthik2804
Copy link
Contributor

Copy link
Owner

@rajatjindal rajatjindal left a comment

Choose a reason for hiding this comment

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

thanks a lot for the PR, the changes look great. I just have a few minor comments.

src/list.rs Outdated Show resolved Hide resolved
src/template.rs Outdated Show resolved Hide resolved
src/template.rs Outdated Show resolved Hide resolved
src/template.rs Outdated Show resolved Hide resolved
src/template.rs Show resolved Hide resolved
src/template.rs Outdated Show resolved Hide resolved
src/template.rs Outdated

pub fn addhelpers(x: &mut Handlebars) {
handlebars_helper!(wrap: |index: usize, s: String| {
wrap_helper(index, "/n".to_string(), s)
Copy link
Owner

Choose a reason for hiding this comment

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

should this be '\n' instead of '/n'?

src/template.rs Outdated
x.register_helper("shuffle", Box::new(shuffle));
}

fn wrap_helper(index: usize, ending: String, s: String) -> String {
Copy link
Owner

Choose a reason for hiding this comment

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

could you please write some unit tests for this function. i tried running this through template and got following error:

thread 'main' panicked at 'attempt to subtract with overflow', src/template.rs:52:20

examples/template.rs

use handlebars::Handlebars;
use handlebars_sprig;

fn main() {
    // Get a handlebars instance
    let mut hbs = Handlebars::new();

    // THE IMPORTANT PART: Add the helpers
    handlebars_sprig::template::addhelpers(&mut hbs);

    // From this point, you can do whatever you want to do with the
    // handlebars instance. It will have all of the functions available
    // to the templates.
    let tpl = "{{ wrap 5 \"this is a string that needs wrapping\" }}";

    // Example of running a template render.
    let empty_context: Vec<String> = vec![];
    println!(
        "Template produced: {}",
        hbs.render_template(tpl, &empty_context).unwrap()
    )
}

Copy link
Owner

Choose a reason for hiding this comment

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

also, please test using unicode characters and verify how the function behaves.

src/template.rs Outdated

while i < s.len() - 1 {
wraploc += 1;
if wraploc >= index {
Copy link
Owner

Choose a reason for hiding this comment

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

i find the code more readable when there are not too many nested if conditions. you can probably write this as:

if wraploc > index {
    i += 1
    continue
}

for k in (0..i).rev() 
.
.
.

@karthik2804
Copy link
Contributor Author

@rajatjindal text wrapping seems to require a bit more logic and testing than what has been presented so far. What would your thoughts be on the deffering adding wrap to a later PR (potentially just pulling in a library)?

@rajatjindal
Copy link
Owner

sounds good to me. we can add that in a later PR

Signed-off-by: karthik Ganeshram <[email protected]>
@karthik2804
Copy link
Contributor Author

This is ready for a rereview

@rajatjindal rajatjindal merged commit 88d05d6 into rajatjindal:main Sep 16, 2022
@rajatjindal
Copy link
Owner

thanks a lot for your PR.

@karthik2804 karthik2804 deleted the feat/add_helpers branch September 16, 2022 17:31
@karthik2804
Copy link
Contributor Author

Would you like to PR the changes into Bartholomew or should I go ahead and make the PR?

@rajatjindal
Copy link
Owner

if you could pls do it, that would be great.

@karthik2804
Copy link
Contributor Author

Will do it today!

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