godotengine
Add initial GDScript formatter along with tests and front-ends
This PR is the first PR in the series of PRs aiming to implement the GDScript formatter within the engine - partially from scratch and partially by reusing https://github.com/godotengine/godot/pull/76211.
Rationale: https://github.com/godotengine/godot/pull/76211#issuecomment-1909043515
In particular, it lays the groundwork for adding GDScript formatting capability to the engine by introducing:
- a very minimal GDScript noop formatter
- unit tests covering noop formatter
- ability to format the code from the editor (from the
Editmenu or viaSHIFT+ALT+Fshortcut) - ability to check file formatting via commandline (e.g. using
./bin/godot.linuxbsd.editor.x86_64 --headless --format --check-only -s script.gd
The in-editor functionality is currently hidden behind an editor setting that is disabled by default.
Tentative plan for followup PRs:
- Add test runner similar to the one for parser that would be able to turn input-output GDScript file pairs (similar to this) into tests.
- Add the most important safety checks to the formatter:
- correctness check that asserts the parse tree before and after formatting is the same (with some minor exceptions)
- stability check that asserts the following -
format_code(format_code(code)) == format_code(code)i.e. once the code is formatted, any further formatting attempt won't change the formatting
- Extend formatter to actually format very simple statements and blocks
- Extend formatter to preserve and format comments correctly and add comment persistence safety check
- Extend formatter to support very simple expressions
- Extend formatter to support basic strategies
- (...)
- Enable formatter by default in the builds without tests and add exposed ~editor~ project settings that can alter the formatter behavior
Add exposed editor settings that can alter the formatter behavior
I think it's important to note obvious thing that formatter rules should be not in editor settings but in separate Resource file (or not Resource, doesn't matter), which is quite important while working with version control systems.
Add exposed editor settings that can alter the formatter behavior
I think it's important to note obvious thing that formatter rules should be not in editor settings but in separate Resource file (or not Resource, doesn't matter), which is quite important while working with version control systems.
Good point - fixed.
How do you plan to do the formatting? Like, I understand that we should start by the beginning and have "a very minimal GDScript formatter with a few hard-coded rules", but currently, the parsing is done with strings, which is to say the least, very lacking.
Right know, this PR sets up a foundation for formatting, but without any foundation for actually format code.
I'm a little bit worried.
How do you plan to do the formatting? Like, I understand that we should start by the beginning and have "a very minimal GDScript formatter with a few hard-coded rules", but currently, the parsing is done with strings, which is to say the least, very lacking.
Right know, this PR sets up a foundation for formatting, but without any foundation for actually format code.
I'm a little bit worried.
Well, I'm planning to deliver PRs within 200-300 LOC so this one brings no actual formatter on purpose - there's just no room for everything.
The formatter draft will be added later on - once the test machinery and safety checks are added. This way the first scenarios where actual formatting happens will be covered by all possible checks from the very beginning.
As for the formatter itself, it will be done basically by traversing the AST. Ideally I'd traverse a parse tree (like I do in https://github.com/Scony/godot-gdscript-toolkit), but in Godot's sources there are no means to get parse tree. Perhaps I'll also utilize the tokenizer (as a helper) since the stream of tokens proven to be parsable may act a bit like a poor's man parse tree (after some cleaning).
Other than this, I'm planning to add formatting strategies at every formatting level so that formatter can try few different approaches such as: format to single line, format to condensed lines, format to as many lines as possible etc. to check which one produces lines within the limit while providing the least amount of lines at the same time.
Ofc. the above plan is tentative - the idea may evolve over time.
Overall, I'm planning to utilize code from https://github.com/godotengine/godot/pull/76211 along with experience I've gained during 5 years of https://github.com/Scony/godot-gdscript-toolkit development. Also - due to the fact that I'm going to deliver really small PRs - I hope to get some help and utilize experience of other Godot contributors along the way as well.
I'd say implementing the framework for a feature separately is not a good workflow, if it's functionality specifically for that one feature. It makes reviewing the framework hard without having the actual needs and contexts of the feature present and clear, it's hard to review based on hypotheticals, and adding code that does nothing is also not optimal IMO
I'd say implementing the framework for a feature separately is not a good workflow, if it's functionality specifically for that one feature. It makes reviewing the framework hard without having the actual needs and contexts of the feature present and clear, it's hard to review based on hypotheticals, and adding code that does nothing is also not optimal IMO
Well, I wouldn't call a few tests and frontends a framework. It's just as small PR with a tiny fraction of feature (in a noop fashion just like @vnen suggested here: https://github.com/godotengine/godot/pull/76211#issuecomment-1934176264) covered by tests and including means to execute. It already works - it formats empty files and files with a single pass statement.
I know it's not much but it's the quality-first approach. Formatter as a feature is basically just like a mathematical function - it must work perfectly. And I'm convinced it should work perfectly from the very beginning as otherwise the chances of success decrease.
@Scony, can you add the GDScript formatter editor functionality behind a project settings flag? Or an editor one. Because I think that we should maybe merge such a feature, but explicitly say that it's experimental and be off by default (for the time being).
@Scony, can you add the GDScript formatter editor functionality behind a project settings flag? Or an editor one. Because I think that we should maybe merge such a feature, but explicitly say that it's experimental and be off by default (for the time being).
Are you sure it makes sense to do it at this point? I anticipate some users may complain on social media and such... I'd at least wait until we have all the statements and simple expressions formattable. Till then I'd leave it as it is now - "enabled only for builds with tests" - so if someone wants to try something, it will be possible yet discouraging.
To be honest, I don't really like the exclusivity of a feature to a non-specific build flag. The TESTS_ENABLED flag is about actual tests, not features.
To be honest, I don't really like the exclusivity of a feature to a non-specific build flag. The
TESTS_ENABLEDflag is about actual tests, not features.
I've been thinking about it and you're right - this will not only be cleaner approach but it will also enable more people willing to help to test formatter in the future.
I've updated PR. I'm just wondering what @akien-mga take will be, but IMO as long as we're in the dev stage and as long as we provide working chunks of formatter, it should be all good.
@Scony Will this work with the Godot language server, so external editors will benefit?
I've updated PR. I'm just wondering what @akien-mga take will be, but IMO as long as we're in the dev stage and as long as we provide working chunks of formatter, it should be all good.
Not sure if this is possible, but is there maybe a way to mark settings as experimental?
@Scony Will this work with the Godot language server, so external editors will benefit?
This PR (among other things) brings the ability to format from the command line, so this can already be attached to editors. As for LSP - I haven't been thinking about it, but it sounds like a good idea for a followup PR.
I've updated PR. I'm just wondering what @akien-mga take will be, but IMO as long as we're in the dev stage and as long as we provide working chunks of formatter, it should be all good.
Not sure if this is possible, but is there maybe a way to mark settings as experimental?
That would be perfect, but I have no clue either - any1 knows if it's possible?
Not sure if this is possible, but is there maybe a way to mark settings as experimental?
That would be perfect, but I have no clue either - any1 knows if it's possible?
Yes, you can mark project and editor settings as experimental/deprecated, they are just properties of ProjectSettings and EditorSettings classes.
As for LSP - I haven't been thinking about it, but it sounds like a good idea for a followup PR.
In general, including this into LSP with this interface is doable, but I just did a quick search in the LSP spec, and the first sentence about formatting documents already mentions client capability tracking which isn't implemented at the moment (or I haven't found it yet :sweat_smile:). So I wouldn't rush it, and aim for a fully compliant implementation, instead of some compromise which ends up working fine with VSCode.
Yes, as part of the handshaking process the client must communicate to the server its capabilities. Does Godot not already do that? I thought it was required to set up a connection
I've updated PR. I'm just wondering what @akien-mga take will be, but IMO as long as we're in the dev stage and as long as we provide working chunks of formatter, it should be all good.
Not sure if this is possible, but is there maybe a way to mark settings as experimental?
Done. Turns out it was as easy as:
</member>
- <member name="text_editor/behavior/formatter/enabled" type="bool" setter="" getter="">
- If [code]true[/code], the experimental code formatter can be used to auto-format GDScirpt files. Currently formatter is not implemented and does not format anything.
+ <member name="text_editor/behavior/formatter/enabled" type="bool" setter="" getter="" experimental="Currently formatter is not implemented and does not format anything.">
+ If [code]true[/code], the experimental code formatter can be used to auto-format GDScirpt files.
Please note that this setting requires editor restart to take an action.
</member>
<member name="text_editor/behavior/general/empty_selection_clipboard" type="bool" setter="" getter="">
Yes, as part of the handshaking process the client must communicate to the server its capabilities. Does Godot not already do that? I thought it was required to set up a connection
I only started looking into the LSP a few weeks ago so take that with a grain of salt, but from what I see, Godot straight up never reads the client capabilities when initializing (can't find a concise version history of LSP, but maybe capabilities weren't a thing in 2019 where this code dates back to :shrug:). The LSP is also currently written in a way which would support multiple clients which isn't intended by the spec, so this isn't exactly straight forward. I was going to write up a proposal about possible LSP refactoring when I find the time, so I'd stop the topic here since it isn't really relevant to this PR.
