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 #37

Merged
merged 10 commits into from Jun 23, 2019
Merged

add go to definition #37

merged 10 commits into from Jun 23, 2019

Conversation

andycraig
Copy link
Contributor

Closes #34

This PR adds Go to Definition functionality.

@Tutuchan added the infrastructure and code to go to definitions of functions in packages; I added code to go to definitions in local files.

The following rules are used:

  1. Go to definition on a function with a pkg:: qualifier will look for that function first in package pkg and then in local files;
  2. Go to definition on a function without a pkg:: qualifier will look for that function first in local files and then in loaded packages.

Note that the language server only becomes aware of functions in local files as those files are registered via didOpen etc. A future feature that registers, e.g., all files in the current project would make Go to Definition more useful.

@Tutuchan I have rebased your commits and my commit onto master.

Running the unit tests using devtools::test() requires that the package already be installed (not just with devtools::load_all()). Otherwise, the unit tests hang. This seems to be the case with master too, though.

@minkir014
Copy link

minkir014 commented Jun 15, 2019

Do you know why the last commit failed????

@minkir014
Copy link

minkir014 commented Jun 15, 2019

And does these packages go to definitions for all packages that are installed??? I mean if I have code like that library(shiny) and I put the mouse over shiny and clicked go to definition. Would it go to definition for that??????

@andycraig
Copy link
Contributor Author

Hi @minkir014, the checks failed for two reasons:

  1. The documentation didn't quite meet CRAN requirements, and
  2. Some unit tests were failing because the language server takes a bit longer to run in Travis CI and I had it requesting data before the data was ready.

Happily, both those issues seem to be resolved now.

For packages, there isn't really a 'definition' to go to. So in the library(shiny) example it wouldn't do anything. But, if you clicked on runExample in shiny::runExample it would go to the definition for runExample.

Copy link
Contributor

@Tutuchan Tutuchan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andycraig this is great work, thank you.

A future feature that registers, e.g., all files in the current project would make Go to Definition more useful.

Yes, this would be great!

Running the unit tests using devtools::test() requires that the package already be installed (not just with devtools::load_all()). Otherwise, the unit tests hang. This seems to be the case with master too, though.

Yes, I don't know why it behaves that way but I noticed the same behavior.

@randy3k
Copy link
Member

randy3k commented Jun 16, 2019

@andycraig excellent work. I will take a look as soon as possible. @Tutuchan feels free too give any comments.

@minkir014
Copy link

minkir014 commented Jun 16, 2019

Note that the language server only becomes aware of functions in local files as those files are registered via didOpen etc. A future feature that registers, e.g., all files in the current project would make Go to Definition more useful.

Do you mean that go to definition won't work until the files are opened for local purpose???

@andycraig
Copy link
Contributor Author

@randy3k @Tutuchan Thanks both! @Tutuchan Your code was very clear and easy to continue on with :)

@andycraig
Copy link
Contributor Author

@minkir014 For functions in local files (not packages), that’s correct. This is the same for other features like Code Completion and Hover.

@kforner
Copy link

kforner commented Jun 17, 2019

Yes, I don't know why it behaves that way but I noticed the same behavior.

It happened to me. I think this is because the R process run by https://github.com/REditorSupport/languageserver/blob/master/R/languageclient.R#L18 does not find the package, since it is not installed. But there is not any error check at this point.
I propose to add a sleep, then a check that the process is still alive, otherwise raise an execption with the error stream content.
In the my docker pull request, I work-around it by first installing the package.

@andycraig
Copy link
Contributor Author

Hi @kforner, thanks for looking at this. I created a separate issue for it (#39).

@randy3k
Copy link
Member

randy3k commented Jun 23, 2019

I don’t think I will have time review it in details in the near future. After a quick skim, I guess we could merge it first and evaluate.

@randy3k randy3k merged commit 29e940d into REditorSupport:master Jun 23, 2019
@andycraig
Copy link
Contributor Author

@randy3k Sounds good. If you have comments in the future, let me know and I'll be happy to make fixes etc. as required 👍

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.

Go to Definition
5 participants