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 capability to jump to MRI C method sources #91

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rmloveland
Copy link

Human-readable explanation of the scope of changes is below:

Overview

This PR adds the ability to (usually!) jump to the C sources of
Ruby methods.

It only works if the user has GNU Global installed and sets a few
Elisp variables to the right values.

If the user doesn't set the variable robe-use-global-p to true,
the behavior of Robe stays exactly as it is now -- none of
the "jump to C source" machinery is engaged.

Summary of code changes

  • Add functions robe-list-c-method-files,
    robe-determine-c-file-for, robe-jump-to-c-source
  • Add variables robe-use-global-p,
    robe-ruby-source-directory, robe-global-command
  • Change the interface of robe-jump-to to add optional
    arguments that allow it to pass in the value of
    thing-at-point for the "jump to C source" machinery. Because
    the arguments are optional, we don't need to change any of
    robe-jump-to's callers
  • Modify robe-jump-to to check the value of robe-use-global-p
    to decide whether to try to jump to C method sources. For
    users who haven't set this variable, Robe works exactly as it
    does today.

Remaining bugs/weird UX

  • Finding the C source of the method is not 100% accurate - some
    of this is due to GLOBAL, some due to the regex search's
    incomplete coverage
  • When jumping back from the C source, you have to hit M-,
    twice for some reason (possibly a ggtags-mode thing)
  • Sometimes you have to enter the module name in the minibuffer
    twice to jump to C-based definitions, I'm not sure why yet

Human-readable explanation of the scope of changes is below:

Overview
--------

This PR adds the ability to (usually!) jump to the C sources of
Ruby methods.

It only works if the user has GNU Global installed and sets a few
Elisp variables to the right values.

If the user doesn't set the variable `robe-use-global-p` to true,
the behavior of Robe stays exactly as it is now -- none of
the "jump to C source" machinery is engaged.

Summary of code changes
-----------------------

- Add functions `robe-list-c-method-files`,
  `robe-determine-c-file-for`, `robe-jump-to-c-source`

- Add variables `robe-use-global-p`,
  `robe-ruby-source-directory`, `robe-global-command`

- Change the interface of `robe-jump-to` to add optional
  arguments that allow it to pass in the value of
  `thing-at-point` for the "jump to C source" machinery.  Because
  the arguments are optional, we don't need to change any of
  `robe-jump-to`'s callers

- Modify `robe-jump-to` to check the value of `robe-use-global-p`
  to decide whether to try to jump to C method sources.  For
  users who haven't set this variable, Robe works exactly as it
  does today.

Remaining bugs/weird UX
-----------------------

- Finding the C source of the method is not 100% accurate - some
  of this is due to GLOBAL, some due to the regex search's
  incomplete coverage

- When jumping back from the C source, you have to hit `M-,`
  twice for some reason (possibly a `ggtags-mode` thing)

- Sometimes you have to enter the module name in the minibuffer
  twice to jump to C-based definitions, I'm not sure why yet
@dgutov
Copy link
Owner

dgutov commented Jul 21, 2016

Hi!

Sorry about the wait. This is an interesting idea, but I don't like some implementation details. Namely, the added optional arguments (could we split it into a pluggable feature that touches the existing code very litte?) and a bit awkward integration with ggtags (ggtags-find-tag doesn't seem like it's doing too much, maybe we could inline the definition, like with robe-list-c-method-files).

Further:

  • xref-push-marker-stack is only available in Emacs 25. If we were to only target that version, maybe it could be a good idea to use Xref and a separate backend for this feature. Unfortunately, the fallback mechanism there doesn't take into account whether the first tried backend returned any results. So something would have to change for that, here or there.
  • It would be cool if instead of hardcoding the sources directory we took into account the currently used Ruby version (via rbenv or rvm). Unfortunately, both of them by default delete the sources after installation, so it would need a fallback.

This doesn't seem like a frequently-requested feature, so I think we can take our time improving it.

@rmloveland
Copy link
Author

Thanks for this thoughtful feedback. Sorry about my late response.

I agree with you 100% - this is a pretty hacky approach.

I'm not sure when/if I will be able to work more on this PR. Please feel free to reject/close if you like.

Hopefully someone gets interested in hacking on this type of feature in the future. I agree with you that it's not really necessary for day-to-day Ruby programming though. :-)

@dgutov
Copy link
Owner

dgutov commented Feb 6, 2018

This might be relevant: pry/pry-doc#82

Once it gets into a release, delegating some of this logic to pry-doc seems like a better choice (and it only requires find plus etags).

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