Godot has been doing a tour de force lately as an open source alternative to Unity. Add support for the growing [Godot](https://github.com/godotengine/godot) engine's GDScript.
Heavily borrowed from python due to GDScript's syntax being heavily borrowed from python. To note, the language is undergoing some breaking changes that will take effect when godot 4 releases, this already should account for syntactical changes and I've already classified some number of currently-keywords as builtins even though godot 4 isn't released yet.
A little test file that I used to make sure I didn't completely break it while making the changes for the things that are different between python and gdscript: 
And the prerequisite upstream pull requests: https://github.com/ScintillaOrg/lexilla/pull/41 https://github.com/universal-ctags/ctags/pull/3194 You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3012
-- Commit Summary --
* Add GDScript file type
-- File Changes --
M ctags/Makefile.am (1) A ctags/parsers/geany_gdscript.c (784) M data/Makefile.am (1) A data/filedefs/filetypes.gdscript (73) M data/filetype_extensions.conf (1) M scintilla/Makefile.am (1) M scintilla/lexilla/include/LexicalStyles.iface (2) M scintilla/lexilla/include/SciLexer.h (1) A scintilla/lexilla/lexers/LexGDScript.cxx (699) M scintilla/lexilla/src/Lexilla.cxx (3) M scintilla/scintilla_changes.patch (1) M src/filetypes.c (1) M src/filetypes.h (1) M src/highlighting.c (10) M src/highlightingmappings.h (29) M src/symbols.c (1) M src/tagmanager/tm_parser.c (11) M src/tagmanager/tm_parser.h (1) M src/tagmanager/tm_parsers.h (3)
-- Patch Links --
https://github.com/geany/geany/pull/3012.patch https://github.com/geany/geany/pull/3012.diff
Just had a brief look at the PR and it looks good to me in principle, just the scintilla part and the uctags part should be merged upstrem before applying in Geany (with the corresponding changes that the upstram projects request). Then, also drop the `geany_` prefix from the parser file name - it just indicates those parsers where we don't use the upstream version yet.
Will update when upstream pulls, the code in this pull request is fairly out of date currently, haven't copied changes into this for a minute
@Davidy22 pushed 1 commit.
03744f42123c58383eacf42b427c4557c6b0aaca Add GDScript file type
Alright, all upstream sources have merged, copied everything back into here. There is one general function added to ctags entry.c that was created for the GDScript ctag that is probably not in a ctags release yet because it's brand new but I put it in the same place that it is in the upstream. Also did the rename, although I notice every other ctag parser is prepended with geany_
Better wait for #2990 and #2991 in case they cause conflicts.
Alright. I'll use the branch in the meantime, see if it crashes me out or anything
@Davidy22 pushed 1 commit.
88882549e04e4bb025ce2ab9e8cacf2932a22557 Merge branch 'master' into gdscript
Alright, got the notification for the merge, went and fixed the one merge conflict
Updated and resolved merge conflicts now that preceding PRs have been merged
@Davidy22 Could you make the patch on top of current master? There was a problem with the update to new Scintilla that caused crashes and your branch crashed because of that too.
I just had a brief look at the pull request. Could you add some unit test that generates all the kinds supported by the parser? Also, I expect the parser generates full scope information - in such case it should be mentioned in the top part of `tm_parser_has_full_context()`.
@Davidy22 pushed 1 commit.
e2e7bc81907b6cca78dcf2842a9ad569921f0688 Add GDScript file type
@Davidy22 pushed 2 commits.
817b788d2eeb9ff70f30b6b4ed0716c757763579 Add GDScript file type 27c402075620e859def31795b658da0618521074 Add test files for GDScript
Oh, is there new stuff? Rebased again, it did pass CI before rebase though, does something there need to be added to tests as well?
There's a couple of test cases in the upstream ctags that were added as part of the upstream addition, I can copy some of them into test/ctags. Added some lines in lines in tm_parser as well.
Thanks. For the unit tests to work though, more work is needed than just copying them to the test directory. You need to: 1. Update `tests/ctags/Makefile.am` 2. Create empty `tests/ctags/<fname>.tags` where `<fname>` is the filename of each of the tests 3. Re-run `configure` 4. Run `make check` which will fail but which will create `<fname>.log` files inside the `tests/ctags` directory 5. Inside `tests/ctags` run `patch -p0 <fname>.log` for each of the files 6. Re-run `make check` to make sure all tests pass 7. Commit the `<fname>.tags` files and the `Makefile.am` change
I'd also suggest editing `tm_parser_enable_role()` and adding something like: ``` case TM_PARSER_GDSCRIPT: return kind == 'c' ? FALSE : TRUE; ``` otherwise both base classes and real classes are shown in the symbol tree (check e.g. `gdscript-inner-class.gd` where you don't want to see the `Reference` tags in the sidebar).
@Davidy22 pushed 2 commits.
f59e52008e93bf3b1585a918bb248bf5d406e383 Add GDScript file type aec2219ca9c8c5ff04e125052f982feb620d179e Add test files for GDScript
Alright done
@Davidy22 pushed 1 commit.
8ea83ccef816ce744b184c26fc2904df38e102f1 Add test files for GDScript
@techee commented on this pull request.
@@ -147,6 +147,9 @@ test_sources = \
cxx11enum.cpp \ cxx11-final.cpp \ cxx11-noexcept.cpp \ + gdscript-inner-class.gd \ + gdscript-modifiers.gd \ + gdscript-no-implicit-class.gd \
Just a nitpicky comment: better to sort the files alphabetically.
The PR looks good to me. I did test it a bit and everything seems to work and while I'm not a Scintilla highlighting expert, the scintilla part looks good too. I believe the PR could be merged.
@elextr Whet do you think?
Not at my dev machine for a couple of weeks so I can't test, so I did a totally unreliable scan of the changes.
I notice that there are changes to files in `ctags/main`. I thought all those were just copied verbatim from uctags, so any changes there will be overwritten next time it is upgraded?
@Davidy22 commented on this pull request.
@@ -147,6 +147,9 @@ test_sources = \
cxx11enum.cpp \ cxx11-final.cpp \ cxx11-noexcept.cpp \ + gdscript-inner-class.gd \ + gdscript-modifiers.gd \ + gdscript-no-implicit-class.gd \
I think it goes hijklmn, so these are already in alphabetical order?
@Davidy22 commented on this pull request.
@@ -147,6 +147,9 @@ test_sources = \
cxx11enum.cpp \ cxx11-final.cpp \ cxx11-noexcept.cpp \ + gdscript-inner-class.gd \ + gdscript-modifiers.gd \ + gdscript-no-implicit-class.gd \
Oh wait somehow I inserted these in a different place on the list than I thought I did, adjusting
@Davidy22 pushed 1 commit.
c8653dbc4001a453a5cef9e5660fe6bbfd72aaae Add test files for GDScript
The changes in ctags/main are changes that were made in uctags very shortly after the point that the latest ctags update was done from. You should be able to find those functions in upstream, and they should still be there after the next upgrade.
The changes in ctags/main are changes that were made in uctags very shortly after the point that the latest ctags update was done from. You should be able to find those functions in upstream, and they should still be there after the next upgrade.
Yeah, I noticed those too and these should be fine.
Ok, otherwise I didn't spot anything S/B fine
@elextr OK if I merge it?
Sorry if I sound too impatient but I'd like to speed-up the ctags parser merging process a little. I really appreciate your desire for Geany stability and I'm the last who wants to introduce some source of instability to Geany but there are many more parsers to be merged (for which I'd like to create pull requests after improving the symbol tree category mapping a little).
I actually think that keeping the code in separate pull requests is bad for Geany release stability because people don't test these regularly and more eyes see more things in master (like in the case of Scintilla where you alone didn't experience the crash).
It's not that I think that every pull request should be merged immediately (I for instance tend to have a 2 week self-correction phase) but if it's something that's generally agreed on thing like that we want to use the upstream parsers or Scintilla and that doesn't require some extensive review (when merging something like a new implementation of a parser, it's basically unreviewable), I'd suggest merging it soon instead of "soon" :-).
OK if I merge it?
thats what my "Ok" above was meant to convey, 4 hours ago, isn't it merged yet, whats the delay :grin:
One more note - once all the new parsers and related tag manager changes are in, I'd like to do a very extensive testing of all the filetypes and check if things display correctly in the symbol tree and if everything works as expected. But it's a loss of time right now when not everything is in the tag manager yet.
thats what my "Ok" above was meant to convey, 4 hours ago, isn't it merged yet, whats the delay 😁
:-)
Merged #3012 into master.
@Davidy22 Thanks!
woo
@Davidy22 and thanks for your patience and perseverance
@techee I don't disagree with most of what you said [above](https://github.com/geany/geany/pull/3012#issuecomment-1013670170), except that an unstable Geany means anybody who tests has to not use as their production tool, and well, that means it won't get much more testing than individual PRs will. Its a damned if we do, damned if we don't situation, sigh.
@techee I don't disagree with most of what you said above, except that an unstable Geany means anybody who tests has to not use it as their production tool, and well, that means it won't get much more testing than individual PRs will. Its a damned if we do, damned if we don't situation, sigh.
True. But with the parser pull requests at least there's pretty much no Geany code affected apart from the mappings and the parsers themselves should be reasonably well tested upstream (the unit tests use fuzzing and valgrind). And we'll run into this in the future releases too - we'll just copy the upstream version of all parsers and I think none of us will have enough time to manually test every single filetype for various input. So for the parsers we'll have to more or less trust the quality of the upstream code.
So for the parsers we'll have to more or less trust the quality of the upstream code.
Yes, I thought we had already agreed on that, even the chief inspector @b4n :-), all that any scan would look for is obvious PR mistakes, like the PR changing unrelated Geany code (like my question above about changes to ctags/main).
[scintilla/lexilla/src/Lexilla.cxx#L191](https://github.com/geany/geany/blob/master/scintilla/lexilla/src/Lexilla.cxx...):
```c &lmFreeBasic, + &lmGDScript, &lmHaskell, ``` "+" looks like mistake. Or I'm wrong?
Oh word, that looks like a diff screwed up somewhere. How does that even happen? Making a pr, how did nothing break for me because of this?
@Davidy22 nothing broke because a unary plus operator applied to a valid type (arithmetic, pointer etc) returns the original unchanged, and `&lmGDScript` is a pointer, so it was harmless.
But for sure fix it.
Oh nice, saved(?) by c idiosyncrasy
github-comments@lists.geany.org