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

Reposition and add file name to compilation error #315

Merged
merged 2 commits into from
Feb 22, 2024
Merged

Conversation

tdroxler
Copy link
Member

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:
image
after:
image

Copy link
Member

@h0ngcha0 h0ngcha0 left a 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)
Copy link
Member

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.

Copy link
Member Author

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) {
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member Author

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) {
Copy link
Member

Choose a reason for hiding this comment

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

[Nit] return sourceInfo

Copy link
Member Author

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

@polarker polarker merged commit 2326a6a into master Feb 22, 2024
8 checks passed
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.

3 participants