nonsequitur / inf-ruby

218 stars 69 forks source link

Patch for ruby-send-definition for a method within a class #140

Closed sharmanr closed 3 years ago

sharmanr commented 3 years ago

The ruby-send-definition command works fine for a standalone method, but cannot be used for a method within a class (because it only sends the region from the “def” to the “end”). What would be nice would be if it sent the definition with the appropriate “class” and “end” lines surrounding it.

This could be done by using a temporary buffer, but then any error messages from that method would not have the right file and line number.

Here is a patch (unified diff format) that preserves the file and line number. Three things are involved.

  1. Three optional arguments are added to ruby-send-region: a prefix string to be sent before the region, a suffix string to be sent after it, and an offset to be added the line number.
  2. A new function enclosed-classes that returns a list of lines containing “class” for all classes that include the given point, nil if there are none.
  3. Command ruby-send-definition is changed to call enclosed-classes. If the result is non-nil, then these class lines are concatenated into a string separated by newlines and used as the prefix string. The suffix string is that many concatenations of the string “end\n”; the line offset is the negative of that number.

inf-ruby-patch.txt

dgutov commented 3 years ago

Hi! It's an interesting idea.

Instead of enclosed-classes (which is unprefixed, BTW, and doesn't seem to support module ...), do you think it would be easier to reuse ruby-add-log-current-method? It handles modules, and singleton methods too.

Or we can improve enclosed-classes, of course.

sharmanr commented 3 years ago

Hi Dmitry,

Thanks for the comments. I hadn’t thought of modules and wasn’t aware of ruby-add-log-current-method. That function unfortunately returns the same string for method a within class b as it does for method a within module b, so it can’t be used as is.

Adding an optional argument to change what it returns (distinguishing between classes and modules) would be possible but doesn’t seem clean.

I think the best solution is to rework enclosed-classes and rename it (sorry!). Would ruby-enclosures be reasonable? I can’t think of a name for “class or module” and ruby-classes-or-modules sounds clumsy!

One question: function ruby-add-log-current-method constructs a useful regexp, module-re, each time it is called, and this would be useful for our new function. Is there any reason it is a local variable and not a file-global defconst that could be reused (if slightly renamed)?

Cheers, Richard

sharmanr commented 3 years ago

One question: function ruby-add-log-current-method constructs a useful regexp, module-re, each time it is called, and this would be useful for our new function. Is there any reason it is a local variable and not a file-global defconst that could be reused (if slightly renamed)?

Sorry, I see now there were two similar regexps, definition-re and module-re; I see now what it was doing.

Richard

sharmanr commented 3 years ago

Here's a better patch. Instead of adding enclosed-classes I put the equivalent code directly into ruby-send-definition. I also caught the case where it was called outside a definition, which would otherwise send garbage to the process. Patch3.txt

dgutov commented 3 years ago

Thanks, this is looking pretty good now.

I was wondering whether it might be a good idea to extract some logic from ruby-send-region and use it directly in ruby-send-definition together with process-send-string instead of adding the three new arguments, but that seems tricky.

Could you re-send the patch as a PR? Or, failing that, as a patch file with a commit message and your name and email in it? You could use git format-patch, for instance.

sharmanr commented 3 years ago

Hi Dmitry.

Well, I’ve never used git except to download source, so this was a bit of a learning experience! I think I’ve done it correctly, creating a fork, making the change, and doing a pull request. Please let me know if it look ok, or give me some clues if I’ve screwed up!!!

Thanks, Richard

On Dec 26, 2020, at 5:48 PM, Dmitry Gutov notifications@github.com wrote:

Thanks, this is looking pretty good now.

I was wondering whether it might be a good idea to extract some logic from ruby-send-region and use it directly in ruby-send-definition together with process-send-string instead of adding the three new arguments, but that seems tricky.

Could you re-send the patch as a PR? Or, failing that, as a patch file with a commit message and your name and email in it? You could use git format-patch, for instance.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nonsequitur/inf-ruby/issues/140#issuecomment-751415387, or unsubscribe https://github.com/notifications/unsubscribe-auth/ASFRXOJUWVOZR24S4NQRRWDSW2G6VANCNFSM4VBS7PQA.

dgutov commented 3 years ago

It looks good, thanks ;-)

dgutov commented 3 years ago

Merged, and closing :+1: