-
Notifications
You must be signed in to change notification settings - Fork 31
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
Reposition and add file name to compilation error #315
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left few minor comments
const errorRegex = /error \((\d+):(\d+)\):\s*(.*)\n\s*(\d+)\s*\|(.*)\n.*\|(.*)\n\s*\|(.*)(?:\n\s*\|(.*)\n\s*\|(.*))?/ | ||
|
||
export function parseError(error: string): CompilationError | undefined { | ||
const match = error.match(errorRegex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Nit] If for some reason this doesn't match the error regex, does it make sense to fall back to the original error? Otherwise I guess we might get regex error below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we throw it here when we use the parsing function.
${spaces} |${this.errorIndicator} | ||
${spaces} |${this.message}` | ||
|
||
if (this.additionalLine1 && this.additionalLine2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: What is additionalLine1
and additionalLine2
? Is it footer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some errors can have a "hint" footer, see this example from the test Those are "real" error generated by our node CompilerSpec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to take the lines separately, because I have to adapt the spaces before the |
.
return line >= sourceInfoWithLine.startIndex && line <= sourceInfoWithLine.endIndex | ||
}) | ||
|
||
if (sourceInfo === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Nit] return sourceInfo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah indeed 😅 It's fixed
This PR takes the error message coming from the node compilation and shift the index, as well as adding the file name to the error.


Received from node:
after: