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

Improve completion of methods arguments #82

Merged
merged 1 commit into from
Mar 11, 2024
Merged

Improve completion of methods arguments #82

merged 1 commit into from
Mar 11, 2024

Conversation

jalvesaq
Copy link
Member

  • Look for methods for second and following classes in objects with multiple classes.
  • Always run code to build completion data as a Neovim job.
  • Build the args_ file right after building the corresponding objls_ file.
  • Add time to build objls_ + args_ to :RDebugInfo.

- Look for methods for second and following classes in objects with multiple classes.
- Always run code to build completion data as a Neovim job.
- Build the `args_` file right after building the corresponding `objls_` file.
- Add time to build `objls_` + `args_` to `:RDebugInfo`.
@jalvesaq
Copy link
Member Author

This pull request should be tried with the branch improve_methods_args from cmp-r.

Building an args_ file was about three times slower than building the corresponding objls_ file. After some optimization, it's now about 5 times faster than before. The big improvement in the performance was due to avoiding calling help() just to get the directory where the package was installed. Now that it's much faster, it is being built right after the objls_ file. This simplifies the code, which now has about 260 fewer lines.

@PMassicotte
Copy link
Collaborator

oh let me try it!

@PMassicotte
Copy link
Collaborator

image

Thas is nice!

@PMassicotte
Copy link
Collaborator

Are we missing some parameters?

image

@jalvesaq
Copy link
Member Author

correlation is for summary.lm, not for print.summary.lm

@PMassicotte
Copy link
Collaborator

oh wow, I still need coffee this morning :)

I have to make some change to a project, I will report back in 1 h ( I will use your PR)

@jalvesaq
Copy link
Member Author

In this example, the R documentation is shared by two functions and the Arguments section combine all arguments of the two functions.

@PMassicotte
Copy link
Collaborator

This feels very fast already

@jalvesaq
Copy link
Member Author

The completion by rnvimserver + cmp-r is almost instantaneous. The only slowness is making the cache files when the library is loaded for the first time.

collapse = clpstr)
luastr <- paste0("{{label = '", luastr, "', cls = 'm', env = '", env, "'}}")
return(luastr)
for (obc in objclass) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nicely done!

@PMassicotte
Copy link
Collaborator

Well, I have worked roughly 1 hour and it works perfectly fine. Speed is not comparable as it was before. You did a very nice job here :) I do not see any problem at mergin this 👍

@jalvesaq
Copy link
Member Author

I ran it with Valgrind and there was no invalid read or write in the C code. So, I believe that it's safe to merge. If any bugs appear, we will fix them.

@PMassicotte
Copy link
Collaborator

I have tried it also on a Window machine and no issue.

@PMassicotte PMassicotte merged commit 2311de6 into R-nvim:main Mar 11, 2024
1 check passed
@jalvesaq
Copy link
Member Author

Thank you!

@PMassicotte
Copy link
Collaborator

Just need to merge the cmp branch

@jalvesaq
Copy link
Member Author

I'll do it... Thanks for remembering!

@jalvesaq jalvesaq deleted the improve_methods_args branch March 11, 2024 19:49
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