Errors logs improvements #167

ilyapuchka · 2017-12-26T14:52:42Z

Original PR #146

Example error logs:

1:9: error: 'for' statements should use the following syntax 'for x in y where condition'.
Hello {% for name in %}{{ name }}, {% endfor %}!
         ^~~~~~~~~~~

invalid-base.html2:24: error: filter error
{% block body %}Body {{ target|unknown }} {% endblock %}
                        ^~~~~~~~~~~~~~

invalid-child-super.html2:25: error: filter error
{% block body %}Child {{ block.super }}{% endblock %}
                         ^~~~~~~~~~~

kylef · 2017-12-27T04:30:02Z

Sources/Context.swift


-  init(dictionary: [String: Any]? = nil, environment: Environment? = nil) {
+  init(dictionary: [String: Any?]? = nil, environment: Environment? = nil) {


I've seen this change in multiple pull requests. Wanna split this out into a separate PR for discussion?

it's in #166 , I just based all other branches on its branch so that tests don't randomly fail, that's why you see them in each pr =)

ilyapuchka · 2017-12-27T12:28:04Z

@kylef I've simplified this a lot by storing filenames and token positions at parsing time instead of calculating them at rendering time, now it looks much closer to what you've started in #82 .

ilyapuchka · 2017-12-27T13:17:28Z

@kylef this started to fail on swift 3.1 with some creepy compiler error. Maybe we should just drop it already?

…en highlighted in error message

…nclude and extend nodes

- moved back to single line string literal - fixed calculating string lenght

this resolves warning related to Never type

DivineDominion · 2018-07-11T10:06:00Z

Whoa this looks cool. I'd like to +1 here so somebody from the team takes a look :)

djbe · 2018-07-11T22:37:53Z

@ilyapuchka Any idea how hard it'd be to rebase/rework this on master for you?
While this may not seem a "flashy" issue with fancy new features, it could end up being the most useful PR of all those that are open right now for template writers.

ilyapuchka · 2018-08-12T21:28:04Z

@djbe I've updated this with the latest master. If you are going to merge any PRs please merge this first.

djbe · 2018-08-13T15:08:46Z

Tests/StencilTests/ForNodeSpec.swift

@@ -98,15 +98,15 @@ func testForNode() {

    $0.it("renders the given nodes while filtering items using where expression") {
        let nodes: [NodeType] = [VariableNode(variable: "item"), VariableNode(variable: "forloop.counter")]
-        let `where` = try parseExpression(components: ["item", ">", "1"], tokenParser: TokenParser(tokens: [], environment: Environment()))
+      let `where` = try parseExpression(components: ["item", ">", "1"], tokenParser: TokenParser(tokens: [], environment: Environment()), token: .text(value: "", at: .unknown))


Indentation?

djbe · 2018-08-13T15:08:53Z

Tests/StencilTests/ForNodeSpec.swift

        let node = ForNode(resolvable: Variable("items"), loopVariables: ["item"], nodes: nodes, emptyNodes: [], where: `where`)
        try expect(try node.render(context)) == "2132"
    }

    $0.it("renders the given empty nodes when all items filtered out with where expression") {
        let nodes: [NodeType] = [VariableNode(variable: "item")]
        let emptyNodes: [NodeType] = [TextNode(text: "empty")]
-        let `where` = try parseExpression(components: ["item", "==", "0"], tokenParser: TokenParser(tokens: [], environment: Environment()))
+      let `where` = try parseExpression(components: ["item", "==", "0"], tokenParser: TokenParser(tokens: [], environment: Environment()), token: .text(value: "", at: .unknown))


Indentation?

djbe · 2018-08-13T15:19:57Z

Sources/ForTag.swift

-      : nil
-
-    return ForNode(resolvable: resolvable, loopVariables: loopVariables, nodes: forNodes, emptyNodes:emptyNodes, where: `where`)
+    return ForNode(resolvable: resolvable, loopVariables: loopVariables, nodes: forNodes, emptyNodes:emptyNodes, where: `where`, token: token)


Space after the :.

djbe · 2018-08-13T15:20:15Z

Sources/ForTag.swift

  }

-  init(resolvable: Resolvable, loopVariables: [String], nodes:[NodeType], emptyNodes:[NodeType], where: Expression? = nil) {
+  init(resolvable: Resolvable, loopVariables: [String], nodes:[NodeType], emptyNodes:[NodeType], where: Expression? = nil, token: Token? = nil) {


Space after the :.

djbe · 2018-08-13T15:23:56Z

Sources/Lexer.swift

+      lineContent = line
+      if let rangeOfLine = self.range(of: line), rangeOfLine.contains(range.lowerBound) {
+        offset = distance(from: rangeOfLine.lowerBound, to:
+          range.lowerBound)


bizarre spacing here?

djbe · 2018-08-13T15:24:35Z

Sources/Node.swift

 }


 /// Render the collection of nodes in the given context
 public func renderNodes(_ nodes:[NodeType], _ context:Context) throws -> String {
-  return try nodes.map { try $0.render(context) }.joined(separator: "")
+  return try nodes.map({


Those () aren't needed for the map.

djbe · 2018-08-13T15:25:55Z

Sources/Parser.swift

+    do {
+      return try FilterExpression(token: filterToken, parser: self)
+    } catch {
+      if var error = error as? TemplateSyntaxError, error.token == nil {


Might be good to use a guard here? It avoids 1 level of indentation.

djbe · 2018-08-13T15:28:42Z

Tests/StencilTests/EnvironmentSpec.swift

@@ -32,9 +39,304 @@ func testEnvironment() {

      try expect(result) == "here"
    }
+
+    func expectedSyntaxError(token: String, template: Template, description: String) -> TemplateSyntaxError {
+      let range = template.templateString.range(of: token)!


Always avoid implicit unwraps. You can replace this with a guard that falls back to an error with less information if the token isn't found. Or a fatalError (as this is a test, might as well break early).

I would prefer failing here as it is crystal clear what is the reason than pollute the code with boilerplate guards especially considering that this is a test and we don't need graceful error handling or whatever.

A guard with a fatalError and a message then. It's not because it's a test that it shouldn't be good code.

I think this code is good enough, but I will do that if that's what it takes to merge this.

@djbe done in 96a004e

djbe · 2018-08-13T15:30:34Z

Tests/StencilTests/EnvironmentSpec.swift

+                     file: String = #file, line: Int = #line, function: String = #function) throws {
+      let expectedError = expectedSyntaxError(token: token, template: template, description: reason)
+
+      let error = try expect(environment.render(template: template, context: ["names": ["Bob", "Alice"], "name": "Bob"]),


Both the declaration of this func as well as this line have some bizarre indentation. This also applies to the similar functions below.

This function is repeated quite a few times. That could be rewritten?

I'd prefer this to 6 arguments stacked on each other
this can be cleared up sometime later, this PR is big without it

djbe · 2018-08-13T15:34:13Z

Tests/StencilTests/LexerSpec.swift

+          "%}{{\n" +
+          "name\n" +
+          "}}{%\n" +
+        "endif %}."


Indentation for this last line.

djbe · 2018-08-13T15:35:19Z

Seeing as you'd rather merge this first, I'll review this one before the others.

ilyapuchka · 2018-08-13T19:00:52Z

@djbe fixed indentations

kylef · 2018-08-13T21:53:31Z

Sources/Environment.swift

+  public init(loader: Loader? = nil,
+              extensions: [Extension]? = nil,
+              templateClass: Template.Type = Template.self,
+              errorReporter: ErrorReporter = SimpleErrorReporter()) {


If I understand correctly, the errorReporter property is only used by context's errorReporter property. But that isn't used anywhere (I may have missed this)?

What is the reason that the Environment and Context should hold the errorReporter? If I understand how this is intended to be used, a consumer of the library would catch an error and pass it to the errorReporter themselves? In that case I think it would make sense to remove the property from Environment and Context leaving the consumer free to create their own ErrorReporter as desired. Decoupling these a little bit.

ilyapuchka · 2018-08-13T22:06:30Z

@kylef this is probably a leftover from when I was using it to render error in inheritance nodes. Not needed now, removed in 92ebfe5

ilyapuchka · 2018-08-14T00:06:05Z

@kylef you'd like to add something?

djbe · 2018-08-15T12:07:56Z

Seeing as there's no more feedback, and this PR is GTM for me, we can go on to the next PRs 🎉

ilyapuchka requested a review from kylef December 26, 2017 14:52

ilyapuchka force-pushed the errors-logs-improvements branch from 0316714 to bf65546 Compare December 26, 2017 15:43

ilyapuchka added this to the 1.0.0 milestone Dec 27, 2017

kylef reviewed Dec 27, 2017

View reviewed changes

ilyapuchka force-pushed the errors-logs-improvements branch from 998a2b5 to 526f05e Compare December 27, 2017 12:24

ilyapuchka force-pushed the errors-logs-improvements branch from 6dbbdcb to e9c2e1c Compare December 27, 2017 12:56

ilyapuchka added a commit that referenced this pull request Dec 28, 2017

storing token in break and continue nodes for error reporter (#167)

d298ff0

ilyapuchka added a commit that referenced this pull request Jan 1, 2018

storing token in break and continue nodes for error reporter (#167)

2c332e6

ilyapuchka added a commit that referenced this pull request Jan 1, 2018

storing token in break and continue nodes for error reporter (#167)

184fab2

ilyapuchka added 19 commits January 1, 2018 16:21

improved template syntax errors with file, line number and failed tok…

6300dbc

…en highlighted in error message

replaced Token with Lexeme protocol on TemplateSyntaxError

69cd8e4

added ErrorReporter protocol with default implementation

0edb385

using error reporter from environment to handle syntax errors

d5f0be9

handling unknown filter errors

e59609f

added parent context to ErrorReporterContext and handling errors in i…

079fdf3

…nclude and extend nodes

renamed Scanner property for original content

7688326

xcode backward compatibility fixes

97ab3cf

- moved back to single line string literal - fixed calculating string lenght

fixed iterating over template lines on linux

5878c32

changer Never return type to Error in ErrorReporter

27135f3

this resolves warning related to Never type

reporting node rendering errors using reference to node’s token

53c1550

reporting error with its parent context

9a28142

fixed reporting errors in child templates

c486617

unified setting higlighting range for errors

bb3f337

fixed highlighting of errors happening in {{ block.super }}

ea7e1ef

updated CHANGELOG

218822f

replaced Lexeme protocol with Token

8d68edd

fixed error on swift 3.1

7756522

refactored environment tests

ed885f4

ilyapuchka force-pushed the errors-logs-improvements branch from f6c0920 to b542927 Compare January 1, 2018 15:22

ilyapuchka mentioned this pull request Jan 2, 2018

Custom filters discoverability krzysztofzablocki/Sourcery#471

Closed

kylef added the rock label Apr 5, 2018

ilyapuchka mentioned this pull request Jun 7, 2018

Feature suggestions krzysztofzablocki/Sourcery#629

Closed

djbe modified the milestones: 1.0.0, 0.12.0 Jul 11, 2018

ilyapuchka added 2 commits August 12, 2018 22:08

Merge branch 'master' into errors-logs-improvements

3a4cd8a

store reference to token when parsing range variable

4f1a5b3

djbe requested changes Aug 13, 2018

View reviewed changes

fixed indetnations

b9702af

more indentations fixed

71ad162

kylef reviewed Aug 13, 2018

View reviewed changes

removed unneeded error reporter references

92ebfe5

replace implicitly unwrapped optional with fatalError

96a004e

djbe approved these changes Aug 14, 2018

View reviewed changes

djbe merged commit ffe8f9d into master Aug 15, 2018

djbe deleted the errors-logs-improvements branch August 15, 2018 12:06

djbe mentioned this pull request Aug 27, 2018

Stencil 0.12 SwiftGen/StencilSwiftKit#95

Merged

yonaskolb mentioned this pull request Sep 2, 2018

Optimise Scanner performance #226

Merged

djbe mentioned this pull request Sep 7, 2018

Attach source maps to tokens and errors #82

Closed

5 tasks

ilyapuchka mentioned this pull request Sep 8, 2018

Performance improvements #230

Merged

djbe mentioned this pull request Sep 22, 2018

Include source maps for template syntax errors #28

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Errors logs improvements #167

Errors logs improvements #167

ilyapuchka commented Dec 26, 2017 •

edited

kylef Dec 27, 2017

ilyapuchka Dec 27, 2017

ilyapuchka commented Dec 27, 2017

ilyapuchka commented Dec 27, 2017

DivineDominion commented Jul 11, 2018

djbe commented Jul 11, 2018

ilyapuchka commented Aug 12, 2018

djbe Aug 13, 2018

djbe Aug 13, 2018

djbe Aug 13, 2018

djbe Aug 13, 2018

djbe Aug 13, 2018

djbe Aug 13, 2018

djbe Aug 13, 2018

djbe Aug 13, 2018

ilyapuchka Aug 13, 2018

djbe Aug 13, 2018

ilyapuchka Aug 13, 2018

ilyapuchka Aug 13, 2018

djbe Aug 13, 2018

djbe Aug 13, 2018

ilyapuchka Aug 13, 2018

djbe Aug 13, 2018

djbe commented Aug 13, 2018

ilyapuchka commented Aug 13, 2018

kylef Aug 13, 2018

ilyapuchka commented Aug 13, 2018

ilyapuchka commented Aug 14, 2018

djbe commented Aug 15, 2018


		init(dictionary: [String: Any]? = nil, environment: Environment? = nil) {
		init(dictionary: [String: Any?]? = nil, environment: Environment? = nil) {

Errors logs improvements #167

Errors logs improvements #167

Conversation

ilyapuchka commented Dec 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyapuchka commented Dec 27, 2017

ilyapuchka commented Dec 27, 2017

DivineDominion commented Jul 11, 2018

djbe commented Jul 11, 2018

ilyapuchka commented Aug 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djbe commented Aug 13, 2018

ilyapuchka commented Aug 13, 2018

Choose a reason for hiding this comment

ilyapuchka commented Aug 13, 2018

ilyapuchka commented Aug 14, 2018

djbe commented Aug 15, 2018

ilyapuchka commented Dec 26, 2017 •

edited