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

Question: adding a kind to exising parser of u-ctags #2227

Closed
masatake opened this issue Jul 24, 2019 · 9 comments
Closed

Question: adding a kind to exising parser of u-ctags #2227

masatake opened this issue Jul 24, 2019 · 9 comments

Comments

@masatake
Copy link
Contributor

Sharing parser code between Geany and u-ctags (= making ctags a library) is in progreass.
(The pull request for sharing was opened by @techee, and I merged them slowly.)

I, a head maintainer of u-ctags, must take care not breaking the interface between Geany and u-ctags when introducing a change to u-ctags.

I got a question about interface when an issue, universal-ctags/ctags#2143, was opened.

Does adding a new kind to an existing parser of u-ctags impact on the interface and Geany behind the interface?

As far as I know, Geany converts u-ctags parser native-kinds to parser neutral abstracted kinds to use the result of u-ctags. Geany may have a table for the kinds conversion.
When I add a new kind to u-ctags, I guess the conversion table must be updated.

@elextr
Copy link
Member

elextr commented Jul 24, 2019

Geany may have a table for the kinds conversion.

Well its a switch but basically the same thing. There are several other switches too.

When I add a new kind to u-ctags, I guess the conversion table must be updated.

Yes, that will always be the case. Whoever makes the pull request on Geany to update the parsers will have to check for such changes, same thing has to be done with Scintilla lexers if they add new syntactic elements.

I would think the best thing is for you to make changes to the shared API clear, so the person doing the import to Geany knows to check for any other changes needed, and perhaps in the longer term have an API version that has to be changed every time the API changes.

@b4n is the symbol expert and may have other suggestions.

@masatake
Copy link
Contributor Author

masatake commented Jul 24, 2019

...I think it is nothing about symbol. (the output of nm doesn't help us, e.g.)

We have to solve the issue by designing work-flows.

I would like to explain a situation causing a trouble.

I will receive a pull request from a volunteer extending a parser which adds a kind to the parser.
If I have to get an agreement Geany people about adding the kind, it will make timer longer for merging. It can be a bottle neck of development of u-ctags. This is MY postion.
In the other hand, Geany people expects Geany works well with newly pulled u-ctags code always.

u-ctags has TLIB test harness which runs mini-geany. TLIB checks u-ctags doesn't break what Geany wants. Actually, adding a kind in a commit is detected by TLIB. A TLIB test case was failed. In that case, what I can do? I contacted @techee, I have not got an answer yet. (I NEVER COMPLAIN about the delay.) In that case, I cannot merge the change. So I opened this issue.

I would like Geany people to allow me to add kinds to existing parsers without "ack from Geany".
The "ack from Geany" can be bottle neck of development. (I'm only talking about adding a kind.
If u-ctags breaks API, I must get "ack from Geany" and as I will write, such breakage must be detected by TLIB test harness.

Instead, I would like Geany people to introduce acceptance test in Geany side.
The acceptance test detects the important changes in u-ctags.
The acceptance test should be run when you updates the u-ctags source tree in Geany.

The acceptance test reports something like this:

[result]
1 new kind `D/maroparm` is added to C parser. 
You must update the SWITCH converting to parser neutral kind.

u-ctags tries not breaking Geany by TLIB test cases. However, it is not enough.
I think Geany side also must have a test accepting the latest u-ctags code.

Writing such test case may be easy. ctags --list-kinds= or ctags --list-kinds-full= may help us. If you are o.k., I will write a prototype and make a pull request.

Thank you for reading.

@elextr
Copy link
Member

elextr commented Jul 24, 2019

...I think it is nothing about symbol. (the output of nm doesn't help us, e.g.)

I think you have misunderstood my comments, "symbols" is what tags are known as inside Geany, the link to the switch I posted above is to the file symbols.c and its the one where @b4n then @techee are the experts (definitely not me) so I noted they may have suggestions.

I was not suggesting you needed to get "permission" from Geany to change the kinds of tags you generate, the projects have very different timelines and resource availability and it may be some time before Geany can address new ctags changes. It is not reasonable to expect ctags to wait for that.

But it would be good if you made it clear that the API change happened. The inclusion of ctags code into Geany is always going to be a manual process because the mapping between the two may need adjusting for any number of reasons.

As I noted this is also the case with the Scintilla code Geany uses, the mapping has to be checked and adjusted manually.

So having to manually check things is not new for Geany devs, but it helps us if changes to the API are highlighted to us. If there is a tool/test that will assist in that even better, but I doubt Geany is likely to have the resources to generate such a tool itself. However if running the previous version of the ctags acceptance tests against a new version will highlight differences then thats useful.

@masatake
Copy link
Contributor Author

Oh, I see.

Defining the API is the best. However, it is not easy and cannot be done now. On the other hand, a new kind is already added to u-ctags source tree.

So I will write a prototype tool for reporting the changes of the API in and send a pull request to Geany. We can use it as the start line of future discussion.

@techee
Copy link
Member

techee commented Jul 24, 2019

@masatake I don't think you have to care about anything when you add a new kind - it's Geany developer's business to make sure everything works. Yes, we have conversion tables for individual kinds but we also check if the number of entries inside the conversion tables matches the number of kinds defined in ctags parsers. Also, we check if the kind letters we have defined in the conversion tables correspond exist in the given parsers. See

https://github.com/geany/geany/blob/master/src/tagmanager/tm_parser.c#L672

When there's a mismatch, Geany refuses to start and writes an error message including the name of the parser.

Finally, we have unit tests for all the parsers which would fail in cases if you e.g. swapped kind letters of different kinds.

So I think we have all we need to detect such changes in parsers and fix them.

@masatake
Copy link
Contributor Author

When there's a mismatch, Geany refuses to start and writes an error message including the name of the parser.

Nice. It is the best way.

@masatake
Copy link
Contributor Author

Thank you for answering.

@b4n
Copy link
Member

b4n commented Jul 26, 2019

Late on my part, but just to support @elextr's point: don't worry about Geany for extending parsers. We definitely don't want slowing down uctags development, and as mentioned we already have what's needed to detect what needs be updated when pulling a new uctags version.

The API tlib should really care about is the part that allows selecting a parser and calling it, e.g. the C API Geany can use; what each parser reports does not really matter but for how useful it is. I guess the breaking of the tlib test case just happens because we gotta check something to see if all is well, not really because we require this output.

@masatake
Copy link
Contributor Author

@b4n, thank you. I understand the situation. I will add comments to the tlib test case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants