edwindj / whisker

{{mustache}} for R
https://mustache.github.io
213 stars 19 forks source link

Template vector, first element empty string does not work #21

Closed gaborcsardi closed 8 years ago

gaborcsardi commented 8 years ago
❯ whisker.render(c("xxx", "foobar"), list())
[1] "xxx\nfoobar"

❯ whisker.render(c("", "foobar"), list())
[1] ""

I suppose I can remove the "" and add one or more \ns to the beginning of the first non-empty string in the character vector as a workaround. But still, this is a bug imo, unless I am missing something.

edwindj commented 8 years ago

Thanks for reporting! I will look into it shortly.

Best

edwindj commented 8 years ago

Hi Gabor,

I cannot reproduce your bug report: I'm getting:

> whisker.render(c("xxx", "foobar"), list())
[1] "xxx\nfoobar"
> whisker.render(c("", "foobar"), list())
[1] "\nfoobar"

Both with the current CRAN version (0.3.2), as well as the latest github version (0.4), both on Ubuntu 15.10.

Any ideas?

gaborcsardi commented 8 years ago

This is what I have in the CRAN version (0.3-2):

> whisker.render
function (template, data = parent.frame(), partials = list(), debug = FALSE)
{
    if (is.null(template) || template == "") {
        return("")
    }
...

which means:

> template <- c("", "foobar")
> is.null(template) || template == ""
[1] TRUE

which is clearly wrong. The GH version is indeed good, sorry, I did not check that: https://github.com/edwindj/whisker/blob/master/R/whisker.R#L22-L24

Btw. that paste there is really inefficient, the "proper" way to write this is:

 if (length(template) == 0 || identical(template, "")) {

Anyway, the GH version seems to be fine. Thanks for looking at it!

EDIT: actually maybe my version is not good, it does not catch c("", ""). So it needs something like all(template == ""), which is almost the same as your paste.

edwindj commented 8 years ago

Thanks for performance tip (I knew it is slow, but it is since longtime on my TODO list to improve performance...).