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 support for Kotlin tags #2778
Conversation
@techee I think we simply pull updated ctags these days to get new parsers, right? |
bdc7bdd
to
b02fc57
Compare
Since the merge of #2666 thats my understanding too, @dolik-rce is your clone up to date enough to include that? |
@@ -7,6 +7,10 @@ AM_CFLAGS = \ | |||
$(GTK_CFLAGS) \ | |||
@LIBGEANY_CFLAGS@ | |||
|
|||
if MINGW | |||
AM_CFLAGS += -DUSE_SYSTEM_STRNLEN=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
This is based on current master (41624c4). 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 However, please correct me if I'm wrong and if this can be simpler. |
Ahh, kotlin parser isn't already in Geany, ok |
1ca5418
to
1bf4c92
Compare
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 universal-ctags/ctags#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. |
Signed-off-by: Jan Dolinar <jan.dolinar@firma.seznam.cz>
f4b17af
to
8eb6e8a
Compare
Another month has passed with no response, but I'm still not giving up my hopes 🙂 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. |
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.
If you look at the commit with the kotlin support, 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.
That is exactly why I separated it in two commits, which you can review separately: 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 ctags infrastructure 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 😁. |
Ok, I have created two separate PRs:
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. |
This PR adds full support for tags for Kotlin language.
update-ctags.sh
, so it can import peg based parsers from ctags.Please let me know if there is anything else that needs to be done in order to merge this into Geany.