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

Add Go To Definition Support #60

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

csenn
Copy link

@csenn csenn commented Nov 22, 2022

No description provided.

@JPercival
Copy link
Contributor

Nice work! At first glance this looks great. We have a release going out today and I don't have time to do a proper review right this second. There's also little bit on reconciliation to do with the work going on here:
https://github.com/DBCG/cql-language-server/tree/feature-more-hover

But I'll try to get it reviewed and merged by the end of the week. :)

@csenn
Copy link
Author

csenn commented Nov 22, 2022

Hey @JPercival, wasn't actually ready to PR this back to your master, was actually thinking this was opened to my fork!

But it is essentially done, just wanted to ask you a thing or two first.

This handles the cases of go to definition for functionRefs and expressionRefs in either a) same file or b) different file.

I was also planning on adding better hover support (at least adding resultType info per node), but sounds like you are working on something there?

The main code I have relevant to that would be an alg that 1) using a visitor searches and collects all nodes that overlap a particular cursor location and 2) sorts those by size and takes the smallest one to use as the "current node". It would be pretty easy to use the same code to grab the current hover position node, and then display whatever hover data needed depending on the node type.

And one question I had on this implementation involves this line: https://github.com/DBCG/cql-language-server/pull/60/files#diff-c867af65827372dc9baa79df976936f21b3e71df405cf9dbdb758ec96466d741R215

I'm trying to find whether a functionRef matches a potential functionDef by comparing the definition args with the functionRef Args one by one. Is DataType.isCompatablieWith() good enough? It might not work in certain casting cases I'm thinking, but seems to be close in a lot of the manual testing I did.

@JPercival
Copy link
Contributor

Just a heads up that we're donating this codebase to the cqframework organization. You'll need to update repo URLs accordingly. Apologies for the delay in reviewing this!

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

Successfully merging this pull request may close these issues.

2 participants