shepherdwind / velocity.js

velocity for js
MIT License
604 stars 144 forks source link

#if and to many newlines #32

Closed DavidWiesner closed 9 years ago

DavidWiesner commented 9 years ago

Hi, at first thanks for this create library!!!

If i use a #if comes after a line with a newline and the statement is also followed by a new line the rendered file contains two newlines. But there should be only one. The same issue i had with the #end statement. If i use a #set statement outside of an #if, the output is like expected, there is only one newline. But an #set statement inside an #if will also add a newline.

Unit-Tests is worth a thousand words :)

    it('multiline: #set multiline', function(){
      var vm = "$bar.foo()\n#set($foo=$bar)\n..."
      assert.equal("$bar.foo()\n...", render(vm)) 
      // all fine here, test pass
    })

    it('multiline: #if multiline', function(){
      var vm = "$bar.foo()\n#if(1>0)\n...#end"
      assert.equal("$bar.foo()\n...", render(vm)) 
      // assert failed, output is: "$bar.foo()\n\n..."
    })

    it('multiline: #set #set', function(){
      var vm = "$bar.foo()\n...\n#set($foo=$bar)\n#set($foo=$bar)"
      assert.equal("$bar.foo()\n...\n", render(vm))
      // all fine here, test pass
    })

    it('multiline: #if multiline #set', function(){
      var vm = "$bar.foo()\n#if(1>0)\n#set($foo=$bar)\n...#end"
      assert.equal("$bar.foo()\n...", render(vm)) 
      // assert failed, output is: "$bar.foo()\n\n\n..."
    })

    it('multiline: #if multiline #set #end', function(){
      var vm = "$bar.foo()\n#if(1>0)...\n#set($foo=$bar)\n#end"
      assert.equal("$bar.foo()\n...\n", render(vm)) 
      // assert failed, output is: "$bar.foo()\n...\n\n\n"
    })

regards, David

shepherdwind commented 9 years ago

This bug is fix now, I just publish the 0.4.7 version. You can try it, If there is other problem, I will take action soon.

So glad to hear from you voice.

Tests is very usefull.

DavidWiesner commented 9 years ago

Year this was a little to much. So this patch remove all double newlines, even i want to have.

I don´t have a locked deep in the code but bug is hiding on an other place. The newlines should only be removed when the hole line is a #*** statement. For example it works for #set statement outside an #if or #foreach but not inside.

shepherdwind commented 9 years ago

Ok, I know where the bug happen now. Before, the asts was lineal, so I can forEach ast element, when find its type to be directives, I will remove the next new line.

But, after sometimes, I change asts structure. So the ast may be an array, when #if and #set come together, trim fail.

shepherdwind commented 9 years ago

How about my new commit?

DavidWiesner commented 9 years ago

Much much better!!! But I´m sorry, still not perfect. I now have problems with newlines and references in a foreach-loop. You can´t solve the newline problem in the preprocessor. The preprocessor don´t know what happen in the logical line before. So for example: Was there a statement, reference or a string at the end of the previous loop? So here is an Unit-Test for that:

        it('multiline: with references', function(){
            var vm = [ 'a',
                        '#foreach($b in $nums)',
                        '#if($b)',
                        'b',
                        'e $b.alm',
                        '#end',
                        '#end',
                        'c'].join("\n");
            assert.equal([  'a',
                            'b',
                            'e 1',
                            'b',
                            'e 2',
                            'b',
                            'e 3',
                            'c'].join("\n"), render(vm, {nums:[{alm:1},{alm:2},{alm:3}],bar:""}))
        })
shepherdwind commented 9 years ago

You unit-test run ok. But when #if end with blank space and newline, such as#if($a) \n $b #end, that condition will fail. And I fix it at 45fd82e.

But I don't know what condition will preprocessor go wrong. Maybe you can give me some other unit-test.

DavidWiesner commented 9 years ago

That was my fault. I have checkout the wrong branch. I can´t find any condition yet were the preprocessor can get wrong. So your patches will work. But there is one another condition that failed: two newlines after statement.

        it('multiple newlines after statement', function(){
            var vm = '#if(1>0)\n\nb#end'
            assert.equal('\nb', render(vm))
        })

just change the var TRIM_REG = /^\s*\n/; to var TRIM_REG = /^[ \t]*\n/; and this will also pass.

shepherdwind commented 9 years ago

You are right, thanks for review my code.

So help me check if there are some other mistakes. If all is fine, I will merge #34 to master now, and pubilsh new version to npm.

After all, thank you give me so much advice, and even the unit-test, so nice.

DavidWiesner commented 9 years ago

You are welcome! Thank you for your work and this great library!