Skip to content

Fix template error line numbers #380

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

Merged
merged 27 commits into from
Jan 29, 2020
Merged

Fix template error line numbers #380

merged 27 commits into from
Jan 29, 2020

Conversation

mattcoley
Copy link
Collaborator

@mattcoley mattcoley commented Dec 12, 2019

Use the line number of when a path was pushed to the pathStack as the line number for the error message rather than the line number of the error within another file.

TODO: much more testing if this approach is approved

new HashMap<>());

assertThat(result.getErrors()).hasSize(1);
assertThat(result.getErrors().get(0).getLineno()).isEqualTo(7);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously this would be on line number 4 of base.html highlighting {% set test = 1 + 1 %} as the error rather than {{ "test"|add(1) }} in the included file.

Copy link
Contributor

@liamrharwood liamrharwood left a comment

Choose a reason for hiding this comment

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

Nice, thanks for tackling this. 🎉

@@ -498,6 +498,13 @@ public int getPosition() {
}

public void addError(TemplateError templateError) {

// fix line numbers not matching up with source template
if (!context.getCurrentPathStack().isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is needed for addAllErrors as well. I'm also wary of any place that might be doing getErrors().add() instead of this method.

@mattcoley
Copy link
Collaborator Author

To break down how this works:
There are two ways that a file context switch can happen:

  • In something like include or extend, the other file is considered part of the original file so all we do is push to the path call stack. When we add an error, we look at the top of the call stack for the line number of the top template.
  • In something like from or import, the other file is independent. A child interpreter is set up and after the file has been processed, the errors are copied over to the parent interpreter. In this case, we adjust the line numbers of the parent during the copy.

@mattcoley
Copy link
Collaborator Author

I am also planning on wrapping the error messages to be clear where the error in the other file occurs.

@mattcoley
Copy link
Collaborator Author

Currently the wrapped error messages look like: Error in file 'tags/includetag/errors/error.html' on line 4: Invalid input for 'add': input with value 'test' must be a number.
I am open for suggestions on how to improve this messaging.

@TheWebTech
Copy link

@mattcoley This looks good. Trying to think of odd edge cases where this is not going to be ideal, but can't think of any.

Minor convenience suggestion. In design manager error output would be cool if the path text linked to the file/open tab in the design manager.

@mattcoley
Copy link
Collaborator Author

We can add a path attribute to the error message so that the DM can link to the other file.

Copy link
Contributor

@liamrharwood liamrharwood left a comment

Choose a reason for hiding this comment

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

I made a bunch of changes to try and address more problems here, particularly around importing, macros, and resolving blocks / extend parents. I added more test cases as well. Let me know what you think.

@@ -75,4 +77,30 @@ public void push(String path, int lineNumber, int startPosition) {

return Optional.of(stack.peek());
}

public int getTopLineNumber() {
if (topLineNumber == -1 && parent != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I added parent stack checks for these new methods. This will help us deal with child interpreters and also is more consistent with the other methods on CallStack.

Node parentRoot = extendParentRoots.removeFirst();
output = new OutputList(config.getMaxOutputSize());

for (Node node : parentRoot.getChildren()) {
lineNumber = node.getLineNumber() - 1; // The line number is off by one when rendering the extend parent
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to reorient the interpreter when processing extend parents, since this happens after normal rendering.

@@ -309,10 +313,14 @@ private void resolveBlockStubs(OutputList output, Stack<String> blockNames) {
OutputList blockValueBuilder = new OutputList(config.getMaxOutputSize());

for (Node child : block.getNodes()) {
lineNumber = child.getLineNumber();
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to reorient the interpreter when resolving blocks.


try (InterpreterScopeClosable c = interpreter.enterScope()) {
interpreter.setLineNumber(definitionLineNumber);
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to reorient the interpreter when evaluating macro functions so we can correctly set the error line numbers from the original imported file. To do this, each MacroFunction will keep track of the line number and position it was defined at in the original file. When it comes time for evaluation, the child interpreter will be oriented at this location.

(This also meant adding setters for lineNumber and position, which were previously readonly.)

@liamrharwood liamrharwood merged commit e501384 into master Jan 29, 2020
@liamrharwood liamrharwood deleted the fix-line-numbers branch January 29, 2020 20:34
Joeoh added a commit that referenced this pull request Feb 10, 2020
* First draft of deferring from tag including macros

* Checkstyle

* Add more tests

* Remove incorrect assert

* Add test that checks deferring macros in depth and update DeferredValue & MacroFunction

* Checkstyle

* Make constructor private and overload instance method

* Fixes #159: Adds dict support for DefaultFilter

* removes undesired code from Filter.java

* removes whitespaces from DateTimeFormatFilter.java

* Fixes Style Check Errors

* Removes stringArgs from DateTimeFormatFilter

* Revert "Fixes #159: Adds dict support for DefaultFilter"

* Changes DefaultFilter to extend AdvancedFilter

* adds PyList support to ForTag

* adds tests for ForTag

* removes escape filter from fortag test

* Fix resoncstruct end to honor trim tags

* Add reconstructImage to MacroFunction

* Add test for reconstructing macro with no trim tags

* Whitespace fix

* Implement safe filter as SafeString and handle SafeString in filters, functions and expressions (#385)

* Start implementing safe filter

* Remove comment about pass-through implementation

* Return var if it's not instance of string instead of throwing

* Add test for pass-through

* Add support for SafeString to all filters which handle Strings

* Remove utils for string reverse filter

* Handle SafeString in truncate function

* Formatting fix

* Add SafeStringFilter interface

* Handle safe strings in filters

* rm trailing space

* Formatting fixes

* Move safeFilter method to Filter IF and remove SafeFilter IF

* rm space

* Change behaviour of Urlize filter to not always return a SafeString

* Code style changes

* Remove unnecessary call to safeString

* Style fix

* Add tests to handle Urlize string being escaped and made safe

* rm hardcoded string

* rm uneeded getValue

* Add SafeString type as str

* Handle SafeString in expressions

Co-authored-by: Joe <Joeoh@users.noreply.github.com>

* Fix template error line numbers (#380)

* Fix line numbers

* add to some more places

* two levels deep test

* Fix case with child interpreter

* Add deprecation

* Add another test

* Update error messages

* Fix up error messages and tests

* Fix case with scopes

* Add check for inherit

* Put everything on the path stack

* always push parent

* remove callstack crud

* Set back path management

* Fix extend lineNo/position and keep track for each block

* cleanup

* Add test for extends

* Add more tests

* Check parent call stack for emtpy and line numbers

* Fix test

* Reorient line numbers when evaluating macros, make sure to pop import path off of stack

* Add test for imported macros

* Reorient line numbers when resolving blocks

* Reorient line numbers when processing extend parents

* Add tests for extends + includes

* Revert "Implement safe filter as SafeString and handle SafeString in filters, functions and expressions (#385)"

This reverts commit a6bea47.

* Implement safe filter as SafeString and handle SafeString in filters, functions and expressions (#394)

* Start implementing safe filter

* Remove comment about pass-through implementation

* Return var if it's not instance of string instead of throwing

* Add test for pass-through

* Handle safestrings and call implementor

* Add tests

* Handle SafeString in filter using kwargs

* Handle safe strings in most simple expressions

* Handle SafeString in IsUpperExp

* Handle SafeString in filters. Allow overriding from within filter

* Formatting fixes

Co-authored-by: jkollmann <48923512+jkollmann@users.noreply.github.com>

* First draft of deferring from tag including macros

* Checkstyle

* Add more tests

* Remove incorrect assert

* Add test that checks deferring macros in depth and update DeferredValue & MacroFunction

* Checkstyle

* Make constructor private and overload instance method

* Fix resoncstruct end to honor trim tags

* Add reconstructImage to MacroFunction

* Add test for reconstructing macro with no trim tags

* Whitespace fix

* rm duplicate statement after merge

Co-authored-by: Manish Devgan <manish.nsit8@gmail.com>
Co-authored-by: Jeff Boulter <jeff@boulter.com>
Co-authored-by: Joe <Joeoh@users.noreply.github.com>
Co-authored-by: Matt Coley <matthewjcoley@gmail.com>
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.

None yet

5 participants