This PR adds full support for tags for Kotlin language.
- It pulls a recently added Kotlin parser from ctags and integrates it into tagmanager. Disclaimer: The parser was written (and is maintained) by me. - It was also necessary to slightly modify the script `update-ctags.sh`, so it can import peg based parsers from ctags. - I also renamed filetypes.Kotlin.conf to filetypes.kotlin, because geany for some reason (not really clear to me at this time), expects it named this way and if it isn't than syntax highlighting doesn't work at all. I hope this will not be a problem for backward compatibility... - Simple test for kotlin tags is added as well.
Please let me know if there is anything else that needs to be done in order to merge this into Geany. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/2778
-- Commit Summary --
* Add support for Kotlin tags
-- File Changes --
M ctags/Makefile.am (3) M ctags/ctags_changes.patch (13) A ctags/parsers/kotlin.c (17280) A ctags/parsers/kotlin.h (22) A ctags/parsers/kotlin_post.h (161) A ctags/parsers/kotlin_pre.h (81) M data/Makefile.am (2) R data/filedefs/filetypes.kotlin (2) M scripts/update-ctags.py (10) M src/filetypes.c (1) M src/filetypes.h (1) M src/symbols.c (12) M src/tagmanager/tm_parser.c (13) M src/tagmanager/tm_parser.h (1) M src/tagmanager/tm_parsers.h (3) M tests/ctags/Makefile.am (1) A tests/ctags/kotlin.kt (22) A tests/ctags/kotlin.kt.tags (12)
-- Patch Links --
https://github.com/geany/geany/pull/2778.patch https://github.com/geany/geany/pull/2778.diff
@techee I think we simply pull updated ctags these days to get new parsers, right?
@dolik-rce pushed 1 commit.
bdc7bdddb1e4c497d3203a60922b1465d8cc5329 Add support for Kotlin tags
@dolik-rce pushed 1 commit.
b02fc57607f747a3a94270949233fc6ad61d1fae Add support for Kotlin tags
I think we simply pull updated ctags these days to get new parsers, right?
Since the merge of #2666 thats my understanding too, @dolik-rce is your clone up to date enough to include that?
@dolik-rce commented on this pull request.
@@ -7,6 +7,10 @@ AM_CFLAGS = \
$(GTK_CFLAGS) \ @LIBGEANY_CFLAGS@
+if MINGW +AM_CFLAGS += -DUSE_SYSTEM_STRNLEN=1
My knowledge of automake is very limited, so I'll better ask: Is this a correct place to define this macro? Or can it be somehow detected automatically by configure or somewhere else?
I think we simply pull updated ctags these days to get new parsers, right?
Since the merge of #2666 thats my understanding too, @dolik-rce is your clone up to date enough to include that?
This is based on current master (41624c411702d29834da0ff926f3c9f7c895bb47). I might have overlooked something, but I believe that the script by @techee from #2666 is only capable of updating parsers that were already included in geany. I.e. if I update the parser in ctags project in the future, it will be also updated here, as soon as someone runs `update_ctags.sh`. But it does not (and probably should not :slightly_smiling_face:) automaticaly pull in all parsers currently supported by ctags. In fact, I believe this is actually first attempt to actually use this functionality. To add a new parser, some manual work is still required (adding the actuall parser, registering it in tag manager, adding to filedefs etc.) which is exactly what I did in this PR.
However, please correct me if I'm wrong and if this can be simpler.
But it does not (and probably should not slightly_smiling_face) automaticaly pull in all parsers currently supported by ctags. In fact, I believe this is actually first attempt to actually use this functionality. To add a new parser, some manual work is still required (adding the actuall parser, registering it in tag manager, adding to filedefs etc.) which is exactly what I did in this PR.
However, please correct me if I'm wrong and if this can be simpler.
Ahh, kotlin parser isn't already in Geany, ok
@dolik-rce pushed 1 commit.
1ca541875ebf7f2bb808b8ccd56f8553cc49eb2b Add support for Kotlin tags
@dolik-rce pushed 1 commit.
1bf4c926455407f75a95836e849316133deb0329 Add support for Kotlin tags
@dolik-rce pushed 1 commit.
f4b17af1c744f6b5714cfef685b3f4c6835d8824 update ctags
Edit: I have updated CTags. There have been some changes in past few days, that concern the kotlin parser as well (mainly switch to new PackCC upstream, see https://github.com/universal-ctags/ctags/pull/2947 for details)
It seems like @techee is not available at the moment... Is there anyone else who could review this?
Any progress on this? It is here for more then a month with no response... Is there any chance someone could at least look at it please?
I'm sorry if I sound too pushy, that is definitely not the intention. I just want to make sure this PR is not forgotten.
Another month has passed with no response, but I'm still not giving up my hopes :slightly_smiling_face:
I have rebased the code to current master, and it's still ready for review. Can you look at it @techee (or anyone else), please?
@dolik-rce unfortunately there are so many changes to the general ctags infrastructure it needs one of the (few) people who know ctags to look at it, the rest of us would spend so much time understanding the basic ctags operation that we simply don't have the time to look at it.
Also I personally don't understand why #2584 changes only 20 files for both tags parsing and a lexer and you change 57 files for just tag parsing?
In fact if your parser requires updated ctags infrastructure features its probably better to make a separate PR for that rather than mix it all in your new language PR.
unfortunately there are so many changes to the general ctags infrastructure it needs one of the (few) people who know ctags to look at it, the rest of us would spend so much time understanding the basic ctags operation that we simply don't have the time to look at it.
I thought the whole point of the import script by @techee was to make the updates of ctags easy enough so anyone can update them at any time. More frequent updates lead to fewer changes to check, resulting in easier reviews.
Also I personally don't understand why #2584 changes only 20 files for both tags parsing and a lexer and you change 57 files for just tag parsing?
If you look at the commit with the [kotlin support](https://github.com/geany/geany/pull/2778/commits/759d56f70e9b1a8a0b26198a867...), you'll see that it is exactly 20 files (7 of which is imported from ctags, 6 in geany code, 3 in tests, and the rest are makefiles and other "infrastructure"). The rest (37 files) comes from updated ctags.
In fact if your parser requires updated ctags infrastructure features its probably better to make a separate PR for that rather than mix it all in your new language PR.
That is exactly why I separated it in two commits, which you can review separately: - [update ctags](https://github.com/geany/geany/pull/2778/commits/8eb6e8ac5154286ae0390b96c79...) - [Add support for Kotlin tags](https://github.com/geany/geany/pull/2778/commits/759d56f70e9b1a8a0b26198a867...)
Posting them as two merge requests didn't really make sense to me, since neither really make sense by itself. But if you prefer it that way, I can surely make another PR for the update only.
Related question: Would you prefer me to update ctags to lowest version required for the kotlin parser, or should I update it to latest stable release? Ctags development is really lively, there has been about 10 new releases since I originally posted this PR.
Yeah, there is always going to be a tension between slow projects and fast ones where the interface is more than can be accommodated by a DLL (also where the dependency isn't distributed as a DLL). Same for Scintilla. In both cases its not just the function call API (that might be easy to make a DLL), but also the syntactic entity/tag types mappings.
IM(NS)HO separate them, if there is a PR that is solely a (not _too_ big) formulaic update to the ctags and all parser tests pass its possible it might get committed by someone other than one of the experts, thats the idea of @techee's approach AFAIK, you don't _have_ to be a ctags expert, just check nothing out of context is changed and accept that uctags is a fairly well managed project.
That of course only works for infrastructure changes that don't cause changes to other parsers, which would appear to be the case here. And I would update to the latest that does not break any other parsers or change their mapping in symbols (and yes you are responsible for checking that, don't throw it on others if you want it to move fast).
When its mixed I can't tell what might be a change that is part of your parser and whats infrastructure when I'm looking at changed files (ie the deltas in context) since github can only show that for the whole PR, not for individual commits AFAICT (grrrr). Again its a case of "don't make it hard to check if you want it to move", nobody has the time available to do big things.
After the infrastructure update then the "Add Kotlin" update becomes relatively easy to check for major obvious things (which is all that it will be unless the reviewer knows your language). Also the more and bigger tests the better as non-[your favourite language here] users will have more confidence to merge it. Sadly most tags tests are pretty scrappy.
__ALLWAYS__ a smaller PR is better is a good mantra :grin:.
Ok, I have created two separate PRs: - Update ctags: https://github.com/geany/geany/pull/2830 - Add kotlin support: https://github.com/dolik-rce/geany/pull/1
Unfortunately, github does not allow me to create the second one in this repository, because it compares two branches in forked repository. But I'll move it here as soon as possible, as explained in the first PR.
I will close this PR, as it is superseded by the two linked above.
Closed #2778.
github-comments@lists.geany.org