Errors need to show correct line numbers #208

Closed
vasikarla opened this Issue Dec 18, 2012 · 26 comments

Comments

Projects
None yet
4 participants

Any syntactical errors in the dust templates needs to show accurate line numbers instead of only line No 1.

@ghost ghost assigned jairodemorais Feb 21, 2013

Contributor

vybs commented Feb 21, 2013

@vasikarla can you provide more details on the issue.

Contributor

jairodemorais commented Feb 22, 2013

hi @vasikarla: I would like start working in this ticket today, could you provide us more details on the issue?

Contributor

vybs commented Feb 25, 2013

@jairodemorais why cant this be reproduced with unit tests? why wait for @vasikarla

Contributor

jairodemorais commented Feb 25, 2013

@vybs I couldn't reproduce it, do you know the steps to reproduce this issue?

Contributor

vybs commented Feb 25, 2013

I can reproduce it.
Are there unit tests for errors on different lines and it showing the correct line number?
Why dont we start from there first?

Contributor

jairodemorais commented Feb 25, 2013

@vybs

what test are you talking about? this?

{
name: "error: whitespaces between the openning curly brace and forward slash in the closing tags not supported",

source: '{# helper foo="bar" boo="boo"} { / helper }',

context: { "helper": function(chunk, context, bodies, params) {return chunk.write(params.boo + " " + params.foo); } },

error: 'Expected buffer, comment, partial, reference, section or special but "{" found. At line : 1, column : 1',

message: "should show an error because whitespaces between the '{' and the forward slash are not allowed in the closing tags"
}

I think that this is the only one that could confuse someone with the error message

Contributor

vybs commented Feb 25, 2013

ir is just testing a single line. How does this help? I am not sure if you understand the problem discussed here.

Contributor

jairodemorais commented Feb 25, 2013

I understand the problem, the error message shows a wrong line number, but I couldn't reproduce it and all the test that tests the error message are working fine.

Contributor

vybs commented Feb 25, 2013

the test has one line, add a test with multiple lines.

Contributor

jairodemorais commented Feb 25, 2013

I know that the test has only one line :p I tried with other tests. for example this:

{
name: "error: test with multiple lines",

source: 'xxx \n yyyy \n pppp \n {# helper foo="bar" boo="boo"} pap\n lol \n { / helper }',

context: { "helper": function(chunk, context, bodies, params) {return chunk.write(params.boo + " " + params.foo); } },

error: 'Expected buffer, comment, partial, reference, section or special but "{" found. At line : 4, column : 1',

message: "should show an error because whitespaces between the '{' and the forward slash are not allowed in the closing tags"

}

This test pass successfully

Contributor

jairodemorais commented Feb 25, 2013

let me clarify something. the Error message shows the line where the tag with error is opened.

For example in the last example, the mistake is in the line 7, here "{ / helper }'," but the error is telling you that the tag "{# helper foo="bar" boo="boo"}" has an error, so it shows you the line 4.

Contributor

vybs commented Feb 25, 2013

try running same tests on rhino as well.

if these non line number 1 tests are not in, do send a PR to add more tests, also try to simulate cases in partials etc ans see how the error behaves.

Sorry Guys for the late response. Hope you got the context of the error. Good to see you guys jumping on it and working to fix it.

@vybs : Donno if you remember me, i was at LinkedIn last year sometime in July when a bunch of folks from Paypal (including Rich) were present at the dust meeting.

Contributor

vybs commented Feb 25, 2013

@vasikarla it would be great if you can help @jairodemorais with test cases.

@jairodemorais : Lemme know if you need any help..

Contributor

jairodemorais commented Feb 25, 2013

@vasikarla I would need some example where the error massage is wrong.

Contributor

jairodemorais commented Feb 25, 2013

I finally found the error. the problem is in the section rule:

section "section"
  = t:sec_tag_start ws* rd b:body e:bodies n:end_tag &{ return t[1].text === n.text;}
  { e.push(["param", ["literal", "block"], b]); t.push(e); return t }
  / t:sec_tag_start ws* "/" rd
  { t.push(["bodies"]); return t }

Example:

Template:

{#s}
{#&2}
{/s}

Dust is going to parse this template in this way:

  • sec_tag_start => {#s (the position here will be line 1, column 3)
  • rd => } (the position here will be line 1, column 4)
  • body => just the carriage return (\n) because this "{#&2}" doesn't match with anything. (the position here
    will be line 1, column 5)
  • bodies => null because this "{#&2}" doesn't match with bodies
  • end_tag => null because this "{#&2}" doesn't match with end_tag.

So dust think that the error is in line 1, column 1 because it couldn't find the end tag.

Contributor

jairodemorais commented Feb 25, 2013

@jimmyhchan @vybs @smfoote @rragan guys, what do you think about this?

Contributor

jairodemorais commented Feb 26, 2013

Guys, I have been working on this issue, I have implemented a possible solution. I wrote a lot of tests but if you want to add more please tell me and I will add them.

you can see the diff here: https://github.com/jairodemorais/dustjs/compare/GH-208

I have changed the section rule, to fail if it doesn't find the end tag, maybe it is not the most beautiful solution but it worked for the tests that I wrote.
I will wait some feedback.

Contributor

jairodemorais commented Feb 26, 2013

@vasikarla can you provide me some test cases where this was working bad in your templates?

Let me look into those scenarios..it was long back but let me get back to you..

Contributor

jairodemorais commented Feb 27, 2013

thanks @vasikarla.

Anyway I think that my solution is not a final solution, I have found some corner cases where it doesn't work.
I was trying to fix a pegjs limitation with hardcoded javascript in the pegjs rules but i don't like this solution.

So I have created an issue (dmajda/pegjs#163) in the pegjs's github repository. this is the link, let's see what @dmajda says.

Contributor

jairodemorais commented Feb 28, 2013

Guys I have updated my previous change to improve the error message. Thanks @otac0n for ur help, it was really useful.

This is the new diff: https://github.com/jairodemorais/dustjs/compare/GH-208

please take a look and tell me what you think.

Collaborator

smfoote commented Mar 12, 2013

@jairodemorais Can you submit a PR for this change?

vybs added a commit that referenced this issue Mar 19, 2013

Merge pull request #245 from jairodemorais/GH-208
Gh-208 - solve the incorrect error line reported in peg
Contributor

vybs commented Apr 14, 2013

release 1.2.2

@vybs vybs closed this Apr 14, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment