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

Add offline script verifying support using custom build of Godot #469

Merged
merged 3 commits into from
May 2, 2022

Conversation

Razoric480
Copy link
Contributor

@Razoric480 Razoric480 commented Mar 25, 2022

  • This refactors the code to split the ScriptVerifier class to make it into a base class, with test() as a virtual function to override, and implements a OfflineScriptVerifier class. Common functions and data have been moved into the ScriptVerifier.

    • LSP based verification has been removed.
  • The OfflineScriptVerifier class depends on the GDScriptParserWrap Reference class, which is not part of Godot by normal. It requires a custom build of Godot. I've uploaded export templates that include this class at https://github.com/Razoric480/godot/releases/tag/3.4.4-parser, which are compiled on my fork https://github.com/Razoric480/godot/tree/raz/gdparser-expose. Including them as a binary data through a git push is a bad idea given the size. I'm unsure of a better way - would be some CI magic of sorts I imagine.

The GDScriptParserWrap class exposes the parse(script_content: String), has_error(), get_error(), get_error_line() and get_error_column() functions by wrapping Godot's GDScriptParser class, which isn't normally visible through scripting. I didn't expose warnings because they are only visible during tools (though we blacklist anything of that severity anyway.)

  • global/... settings removed.
  • References to the LSP mostly removed, files and classes renamed in consequence, like LanguageServerError just being ScriptError and LanguageServerRange becoming ErrorRange.
  • References and UI related to connecting to a server removed.

Close #300
Fix #310

@NathanLovato
Copy link
Contributor

Thanks for your work @Razoric480, this is a massive improvement.

May I ask you to remove the option to switch entirely, and rather remove the LSP part? It'll still be in the code history should we ever need it (we could keep it in a branch), but the idea's to get rid of it entirely. I can help with removing UI bits like the server connection indicator.

@Razoric480
Copy link
Contributor Author

@NathanLovato Done - I've excised 99-ish% of LSP-ness from the project. Preliminary test reveals it's still working, though users have a knack for finding issues so we'll have to see if I messed something up.

@Razoric480 Razoric480 force-pushed the raz/offline-verifier branch from e39e6d7 to 6397632 Compare March 29, 2022 14:22
@NathanLovato
Copy link
Contributor

Thank you very much François, great work you did there, as usual :)

I'm a bit busy preparing the course release but will test and merge as soon as I'm through with my todo.

@NathanLovato
Copy link
Contributor

After testing, it's working like a charm. I'll address some bugs in the app today, make a stable release, then we can merge this and update the CI for the following release.

@Razoric480 Razoric480 force-pushed the raz/offline-verifier branch from 6397632 to c736d2a Compare May 1, 2022 14:52
@Razoric480 Razoric480 force-pushed the raz/offline-verifier branch from ae2f7a5 to d933549 Compare May 1, 2022 20:05
@NathanLovato NathanLovato merged commit e61ca98 into main May 2, 2022
@NathanLovato NathanLovato deleted the raz/offline-verifier branch September 29, 2023 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants