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

fix: map errors from formatting generated files back to their locations in the templ file #737

Merged
merged 14 commits into from
May 22, 2024

Conversation

Robert-Kolmos
Copy link
Contributor

This is not a complete fix for the original issue #726. The generator does not run the resulting generated files through the go compiler so it is still possible to write a templ where the resulting go file doesn't compile but no error is output when running templ generate. I'd be happy to look into that as well but I wanted to put out a more minimal PR and get feedback first.

templ test() {
    for _ {
        <div>
    }
}

This PR makes 2 main changes

  1. It attempts to map the locations in any errors that the formatter outputs back to their locations in .templ files. If this fails it falls back to reporting the unmapped error.
  2. It uses a different error message to distinguish between when the reported error is in the original .templ file or the generated file.

Copy link
Owner

@a-h a-h left a comment

Choose a reason for hiding this comment

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

Thanks for looking at this!

return fmt.Errorf("%d:%d%s", pos.Line+1, pos.Col, suffix), true
}

func parseFormatterError(str string) (errLine, errCol uint32, suffix string, err error) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's better to inspect the error type.

https://go.dev/play/p/YR6TgOR9XeR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is way better, thank you. I updated the code. Let me know what you think about the error formatting I'm using now.

@@ -221,7 +222,12 @@ func (h *FSEventHandler) generate(ctx context.Context, fileName string) (goUpdat

formattedGoCode, err := format.Source(b.Bytes())
if err != nil {
return false, false, nil, fmt.Errorf("%s source formatting error: %w", fileName, err)
mapped, ok := mapFormatterError(err, sourceMap)
Copy link
Owner

Choose a reason for hiding this comment

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

As per the other comment, I think it would be better to look at the type of the error, and maybe even to rewrite the error that comes back.

A small test around that behaviour would be useful too, so we don't accidentally lose the behaviour in future work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I'm stepping away for a bit and wanted to commit what I've got so far but I will add tests soon.

@Robert-Kolmos Robert-Kolmos changed the title Map errors from formatting generated files back to their locations in the templ file fix: map errors from formatting generated files back to their locations in the templ file May 15, 2024
}{
{
name: "single error outputs location in srcFile",
fileName: "single_error.templ.error",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The .error suffix was necessary to prevent the test files from breaking the build because they would get pulled into any templ generate running from the root.

@Robert-Kolmos Robert-Kolmos requested a review from a-h May 22, 2024 01:56
Copy link
Owner

@a-h a-h left a comment

Choose a reason for hiding this comment

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

Looking good! Thanks for this. 😁

@a-h a-h merged commit b7a4eba into a-h:main May 22, 2024
4 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.

None yet

2 participants