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

text/template: give position info on error #3188

Closed
bytbox opened this issue Mar 4, 2012 · 11 comments
Closed

text/template: give position info on error #3188

bytbox opened this issue Mar 4, 2012 · 11 comments
Assignees
Milestone

Comments

@bytbox
Copy link
Contributor

@bytbox bytbox commented Mar 4, 2012

Errors should include position info whenever possible, to make debugging templates less
reminiscent of tracking down segfaults without a debugger or core dump.
@bytbox
Copy link
Contributor Author

@bytbox bytbox commented Mar 4, 2012

Comment 1:

(This applies to html/template as well, of course.)
@dsymonds
Copy link
Member

@dsymonds dsymonds commented Mar 4, 2012

Comment 2:

Labels changed: added priority-later, packagechange, removed priority-triage.

Owner changed to @robpike.

Status changed to Accepted.

@robpike
Copy link
Contributor

@robpike robpike commented Mar 4, 2012

Comment 3:

I could do without the editorializing, but sure, this won't be too hard. The trick is to
integrate it well.
@moraes
Copy link
Contributor

@moraes moraes commented Jul 31, 2012

Comment 4:

I took a look at this and have a proposal to include filename (if available) and column
number for template errors. Example:
    _, err := template.New("foo").Parse(`
    
        {{if $foo}}{{end}}
    
    `)
Current error: 
    template: foo:3: undefined variable "$foo"
New error:
    template: foo:3:14 undefined variable "$foo"
For this, errorf now also accepts an argument for the input offset (taken from
item.pos), and the template filename is stored for debug purposes when available. My
plan is to remove lex.lineNumber() and rely only on item.pos for error messages.
Please let me know if it is acceptable:
    http://golang.org/cl/6442067
Note: I only implemented it in parse.useVar, so if you think it looks good I can extend
to the rest of the package.
@moraes
Copy link
Contributor

@moraes moraes commented Aug 1, 2012

Comment 5:

Updated with a full implementation: all errors display line/column numbers of the
related token and, when available, the filename.
http://golang.org/cl/6442067
@moraes
Copy link
Contributor

@moraes moraes commented Aug 1, 2012

Comment 6:

Not so fast. Exec errors still don't tell the position.
I'm thinking it'd be better to make lex.position() to return scanner.Position, and nodes
would store the position instead of simply the line number. Then execution errors would
have the full position info like parse errors.
@moraes
Copy link
Contributor

@moraes moraes commented Aug 1, 2012

Comment 7:

A new patch added position info to all exec errors as well. 
Because this replaces the Line field from nodes by a Pos field with scanner.Position,
this is not backwards compatible anymore (and broke html/template), I realized later.
I'll review this and make it fully compatible, keeping existing fields and using a
method for the position info.
Sorry for the comment spam. :P
@robpike
Copy link
Contributor

@robpike robpike commented Aug 1, 2012

Comment 8:

As explained in http://golang.org/doc/contribute.html, we prefer to
discuss significant changes by e-mail before sending code, let alone
submitting a code review through the issue reporting channel. Please
start a conversation in the regular forum to discuss what you'd like
to do. Once we have design we're happy with we can move to reviewing
code.
-rob
@moraes
Copy link
Contributor

@moraes moraes commented Aug 1, 2012

Comment 9:

Thanks. Coding some proof of concepts helped me to understand the problem and write a
proposal to improve this. I've sent it to the list now.
@rsc
Copy link
Contributor

@rsc rsc commented Sep 12, 2012

Comment 10:

Labels changed: added go1.1maybe.

@robpike
Copy link
Contributor

@robpike robpike commented Oct 3, 2012

Comment 11:

This issue was closed by revision 7f4b4c0.

Status changed to Fixed.

@bytbox bytbox added fixed labels Oct 3, 2012
@rsc rsc added this to the Go1.1 milestone Apr 14, 2015
@rsc rsc removed the go1.1maybe label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.