test: log parse error with line context #4

Merged
nikku merged 2 commits from log-context into master 2020-06-14 17:46:36 +02:00
nikku commented 2020-06-12 11:56:08 +02:00 (Migrated from github.com)

Fail with context when failing to parse spec:

Error: Unexpected file format in test-error.txt around

  | # Broken Spec
  |
  | b
  |

Related to https://github.com/lezer-parser/lezer/issues/25

Fail with context when failing to parse spec: ``` Error: Unexpected file format in test-error.txt around | # Broken Spec | | b | ``` Related to https://github.com/lezer-parser/lezer/issues/25
marijnh (Migrated from github.com) reviewed 2020-06-12 18:49:09 +02:00
@ -133,0 +143,4 @@
return file.substring(index, endIndex).split(/\n/).map(str => ' | ' + str).join('\n');
}
marijnh (Migrated from github.com) commented 2020-06-12 18:49:09 +02:00

Why the | markers? I think showing a slice like this, without even taking care to round up/down to line boundaries, produces rather crude output. Wouldn't just outputting the line number be more useful?

Why the `|` markers? I think showing a slice like this, without even taking care to round up/down to line boundaries, produces rather crude output. Wouldn't just outputting the line number be more useful?
nikku (Migrated from github.com) reviewed 2020-06-12 19:31:54 +02:00
@ -133,0 +143,4 @@
return file.substring(index, endIndex).split(/\n/).map(str => ' | ' + str).join('\n');
}
nikku (Migrated from github.com) commented 2020-06-12 19:31:54 +02:00

Well, the output looks exactly as shown in the PR description which I personally find helpful.

Showing line numbers unfortunately won't work, as you (or really anyone) can fiddle around with the file contents before passing it to the function. We do it here, for instance to remove the grammar to be parsed.

As for the rounding, it is not necessary. The regular expression will always match line boundaries so the start index always refers to the start of a line.

Well, the output looks exactly as shown in the PR description which I personally find helpful. Showing line numbers unfortunately won't work, as you (or really anyone) can fiddle around with the file contents before passing it to the function. We [do it here, for instance](https://github.com/lezer-parser/lezer-generator/blob/master/test/test-cases.ts#L28) to remove the grammar to be parsed. As for the rounding, it is not necessary. The regular expression will always match line boundaries so the start index always refers to the start of a line.
marijnh (Migrated from github.com) reviewed 2020-06-12 20:50:21 +02:00
@ -133,0 +143,4 @@
return file.substring(index, endIndex).split(/\n/).map(str => ' | ' + str).join('\n');
}
marijnh (Migrated from github.com) commented 2020-06-12 20:50:21 +02:00

Good point about the line numbers and the start of line. If you add some logic that scans forward to the next line end when there is one or the end of the string otherwise this looks good.

Good point about the line numbers and the start of line. If you add some logic that scans forward to the next line end when there is one or the end of the string otherwise this looks good.
nikku (Migrated from github.com) reviewed 2020-06-12 22:45:42 +02:00
@ -133,0 +143,4 @@
return file.substring(index, endIndex).split(/\n/).map(str => ' | ' + str).join('\n');
}
nikku (Migrated from github.com) commented 2020-06-12 22:45:42 +02:00
https://github.com/lezer-parser/lezer-generator/pull/4/commits/be4aa4e860eedb31dae6c580eedd7d2e7f0b0834 ensures we print full 80 chars + end of last line.
marijnh commented 2020-06-14 17:46:41 +02:00 (Migrated from github.com)

Thanks!

Thanks!
nikku commented 2020-06-30 13:18:57 +02:00 (Migrated from github.com)

Can we cut a release with this change and the other ones recently introduced?

Working of master seems not a good mid-term solution.

Can we cut a release with this change and the [other ones recently introduced](https://github.com/lezer-parser/lezer-generator/compare/0.9.0...master)? Working of `master` seems not a good mid-term solution.
marijnh commented 2020-07-08 08:13:47 +02:00 (Migrated from github.com)

Sorry for the slow turnaround—was on holiday. I've released a 0.9.1 with the new patches.

Sorry for the slow turnaround—was on holiday. I've released a 0.9.1 with the new patches.
nikku commented 2020-07-08 08:24:43 +02:00 (Migrated from github.com)

👍

:+1:
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
lezer/generator!4
No description provided.