Skip to content

Conversation

@DaughterOfZaun
Copy link

@dalexeev
Copy link
Member

Thanks for your contribution! I see it's still a work in progress, but I just want to say that I think you've chosen a suboptimal implementation. There's no need to add to_dictionary() method to every AST node, much less Object.

gdscript_parser.h is already too big. Besides, GDScript contributors use it as a reference for understanding the AST structure. We shouldn't clutter it with implementations or even just declarations of to_dictionary().

Instead, we can use an approach similar to TreePrinter and GDScriptEditorTranslationParserPlugin. But I wouldn't include it in gdscript_parser.{h,cpp}, and I'd factor out TreePrinter as well. Basically, we already have an AST dumper, but the output is not very parsable. We need JSON or something like that.

@DaughterOfZaun DaughterOfZaun force-pushed the script-dump-ast-cmdline-arg branch from 6ae9a6b to 623807a Compare March 22, 2025 09:31
@DaughterOfZaun DaughterOfZaun force-pushed the script-dump-ast-cmdline-arg branch 2 times, most recently from ee946c3 to 473e88b Compare March 30, 2025 16:44
@DaughterOfZaun
Copy link
Author

I definitely need help with this because I don't really understand what I'm doing.

  • I'm not sure I'm calling the parser at the right time during initialization. Maybe it could be done earlier to make it faster.
  • It would also be cool to (optionally) run the analyzer to infer types, but my attempts to make it work failed.
  • It might be better to print the tree directly without converting to dictionaries, but I'm not sure if it's worth the trouble.

In any case, I consider this pull request a temporary measure, because I am convinced that the formatter and GDScript-to-CPP converter should exist in the editor - there is no need for AST export.

@DaughterOfZaun DaughterOfZaun force-pushed the script-dump-ast-cmdline-arg branch from 473e88b to 50c711f Compare March 30, 2025 17:35
@DaughterOfZaun DaughterOfZaun marked this pull request as ready for review March 30, 2025 17:35
@DaughterOfZaun DaughterOfZaun requested review from a team as code owners March 30, 2025 17:35
@DaughterOfZaun DaughterOfZaun force-pushed the script-dump-ast-cmdline-arg branch from 50c711f to cdf720f Compare April 6, 2025 21:04
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.

Add --script-dump-ast command line argument to output an AST of a GDScript file

2 participants