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

Improve error message in text resource format parser #86350

Conversation

Eoin-ONeill-Yokai
Copy link
Contributor

@Eoin-ONeill-Yokai Eoin-ONeill-Yokai commented Dec 20, 2023

Related to my debugging of #77007 and should help users identify potential recursion points in order to circumvent or fix these issues with a work around while we continue to find a fix on an engine level.

This improves the error message in our text resource parsing code to help the user potentially fix parsing issues in case of failure. It also helps with the debugging process of finding out which sub_resource is causing the parser to fail with line messages and resource paths.

Old error message:

ERROR: res://scenes/objects/some_entity.tscn:203 - Parse Error: 
   at: _parse_node_tag (scene/resources/resource_format_text.cpp:284)

New error message:

ERROR: Parse Error: Busy. [Resource file res://Character.tscn:10]
   at: _parse_node_tag (scene/resources/resource_format_text.cpp:285)

This notably:
1 - Gives us the error code, which can help identify which conditional down the code path is flagging an error. It can also potentially help us determine whether or not the error is valid (In this example, the error code is Busy).
2 - Tells the user which file is problematic and which line the parser failed on. The line of failure is important as it empowers the user to find a work around in the worst case scenario, but also helps them find potential mistakes that they may have made during a git conflict resolution.

I'll continue to look into causes for the bug but I think we should consider making our error message here more verbose to help with the debugging process of future tscn parsing bugs. :)

@dalexeev dalexeev requested a review from a team December 20, 2023 07:12
Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

Besides the discussion about error_string vs error_names, LGTM.

This improves the error message in our text resource parsing code to
help the user potentially fix parsing issues in case of failure. It also
helps with the debugging process of finding out which sub_resource is
causing the parser to fail with line messages.
@Eoin-ONeill-Yokai Eoin-ONeill-Yokai force-pushed the text_resource_parse_error_reporting branch from e58eb9c to 2283e07 Compare January 2, 2024 23:00
@akien-mga akien-mga merged commit 03b1934 into godotengine:master Jan 3, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants