nonsequitur / inf-ruby

218 stars 69 forks source link

Usage of inf-ruby with Pry session exposed by RSpec for evaluation of buffer content #184

Closed x3qt closed 6 months ago

x3qt commented 7 months ago

Hello!

I am trying to figure out how I could achieve what title describes using inf-ruby, ie to make ruby-send-region to use Pry session exposed by RSpec. Being newbie to ELisp, I've looked through the source code and decided to see what happens if I add RSpec buffer to inf-ruby-buffers list by running following in IELM:

(setq inf-ruby-buffers (list (get-buffer "*rspec-compilation*")))` 

As a result, as expected, weird stuff happened – whenever I am typing into RSpec buffer myself evaluation happens in the context of a breakpoint, but whenever I am using ruby-send-region, somehow in the context of main object, as depicted on this screenshot:

Screenshot 2024-04-27 at 21 38 19

So, is there a way to achieve desirable behaviour of ruby-send-region, ie to make evaluation to happen in the context of breakpoint, like append-buffer with return command were used and their output shown in a tooltip?

PS. spacemacs with default ruby layer is used

dgutov commented 7 months ago

I suppose you could try altering the value of inf-ruby-eval-binding as well?

There are two questions here:

x3qt commented 7 months ago

Thanks a lot for such fast response!

The main use case I have in my mind is to imitate seeing_is_believing, using RSpec to prepare context and input, run the spec and to start Pry to evaluate things in local context as I write / inspect them, which is now possible thanks to your advice!

Inside of inf-ruby-eval-binding I have replaced Pry.toplevel_binding with just binding to make it local.

To sum it up, two things were required to achieve described behaviour:

(setq inf-ruby-buffers (list (get-buffer "*rspec-compilation*")))
(setq inf-ruby-eval-binding "(defined?(IRB.conf) && IRB.conf[:MAIN_CONTEXT] && IRB.conf[:MAIN_CONTEXT].workspace.binding) || (defined?(Pry) && binding)")

Many thanks once again!

x3qt commented 7 months ago

@dgutov regarding the second point

How to ensure that the added behavior makes sense - i.e. okay, here you evaluate an expression near the binding.pry statement (near the breakpoint, so to speak). But what if you evaluated a line in another class/another file? What if you evaluated a method in another class? It's not a given that the evaluation context should be what the pry is showing, in every such case.

I think this is solvable by using convention of prepending namespace definitions with top-level indicator, ie ::, so whenever such buffer / class / module is sent to REPL it gets evaluated in a global context. (could even be a Ruby Cop)

x3qt commented 7 months ago

@dgutov little demo https://www.youtube.com/watch?v=WnBaMcH5AoY

Seems like a super legit workflow to me, could you please consider mainlining / making it configurable without monkey-patching Emacs runtime?

x3qt commented 7 months ago

Also it would be super awesome to have something like ruby-send-exit, so exit or next breakpoint could be reached without switching to REPL and typing command manually into it, something like this:

(comint-send-string "*rspec-compilation*" "exit\r")
dgutov commented 7 months ago

Seems like a super legit workflow to me, could you please consider mainlining / making it configurable without monkey-patching Emacs runtime?

It's a very valid request, but the question is how to reconcile it with existing behavior.

A ruby-send-exit is easy to do (and it might be useful for inf-ruby buffers as well). But for the rest:

Suppose you launch an rspec compilation which enters pry breakpoint, but at the same time there is an existing inf-ruby repl buffer in the same Emacs session. Which one would be used for evaluation?

And about inf-ruby-eval-binding - the current complex expression hails back to ed768ce4138a by @nonsequitur. The message refers to "consistent access to REPL-local variables". I'm not 100% sure which is the main scenario that change had in mind, but being consistent wrt the scope which is used has its benefits. I guess the main one that you mentioned is references to top-level vs nested constants - which should be solvable by a top-level :: qualifier, except such a reference could already be inside a nested namespace, and it's not trivial to do this conversion for complex expressions, code-wise.

On balance, I'd be inclined to just switch to using binding, but I'm probably missing some scenarios.

x3qt commented 7 months ago

Suppose you launch an rspec compilation which enters pry breakpoint, but at the same time there is an existing inf-ruby repl buffer in the same Emacs session. Which one would be used for evaluation?

I would say the newest one may be used for evaluation, but since it's a switch of context confusion may happen to uninformed users.

Frankly, I don't know what is the typical scenario for having top-level REPL, since as I saw most people develop software which expects some input, this gets covered by a spec runner, and for all-in-one scripts there's something like https://github.com/JoshCheek/seeing_is_believing. I personally used top-level REPL with https://github.com/dgutov/robe until now, but looks like Pry session produced by RSpec works well with Robe too.

x3qt commented 7 months ago

This is how I have it so far:

(defun ruby-start-pry ()
   (interactive)
   (insert "binding.pry")
   (rspec-verify)
   (setq inf-ruby-buffers (list (get-buffer "*rspec-compilation*")))
   (setq inf-ruby-eval-binding "(defined?(IRB.conf) && IRB.conf[:MAIN_CONTEXT] && IRB.conf[:MAIN_CONTEXT].workspace.binding) || (defined?(Pry) && binding)"))

(defun ruby-send-exit ()
   "Send 'exit' to the inferior Ruby process"
   (interactive)
   (ruby-send-string "exit"))

For the ruby-send-exit I've created https://github.com/nonsequitur/inf-ruby/pull/185 since it may be useful outside my use case as well.

x3qt commented 7 months ago

A few things are missing from master branch to allow zero-configuration flow as thread describes:

  1. Pry session exposed by RSpec is not automatically added to inf-ruby-buffers despite *rspec-compilation* buffer entering inf-ruby-mode automatically.
  2. Newest buffer added to inf-ruby-buffers should be used for evaluation.
  3. Context of evaluation should be context of newest buffer at it's binding
dgutov commented 7 months ago

I would say the newest one may be used for evaluation, but since it's a switch of context confusion may happen to uninformed users.

We're not really maintaining a most-recently-seen list of repls in the list, though. It might also be difficult to ensure that it's removed from the list when killed (although I suppose something could be done using some hook). But what if the user explicitly switches to an existing REPL buffer first?

Maybe inf-ruby-proc should look around visible windows, see one showing a buffer in inf-ruby-mode, and return it first, if one exists.

Frankly, I don't know what is the typical scenario for having top-level REPL, since as I saw most people develop software which expects some input, this gets covered by a spec runner

To be frank, I don't use this feature myself very often. But inf-ruby has existed well before both Robe and RSpec, so I think it'd be good to take some care to retain its basics. And, well, I did do some REPL-driven programming on an interview a few months ago, that felt kind of nice.

Worst case, you'll need to (setq inf-ruby-eval-binding "binding") in your config. But hopefully we can puzzle out the original goal of the change and do something compatible.

x3qt commented 7 months ago

We're not really maintaining a most-recently-seen list of repls in the list, though. It might also be difficult to ensure that it's removed from the list when killed (although I suppose something could be done using some hook).

I think that hook could be avoided. If newly spawned inf-ruby-mode buffer will append itself automatically to inf-ruby-buffers and the commands will be sent to the latest appended we probably could check for result, and if receiver is non active then remove last buffer from the list and retry with previous item until nothing left in the list in case of all processes being terminated, like with LIFO stack. This option feels like an easy and simple one.

But what if the user explicitly switches to an existing REPL buffer first? Maybe inf-ruby-proc should look around visible windows, see one showing a buffer in inf-ruby-mode, and return it first, if one exists.

Not sure that the last visited, instead of spawned, should be made active. In my use case, for example, REPL buffer is never visited at all, since it's being spawned by RSpec and out of focus all of the time and commands from current buffer are being sent there and results are displayed in a tooltip. Ideally REPL should be fully hideable. The exit out of this situation is manual REPL selection, ie hard and complex option, basically to have something like Helm prompt.

Worst case, you'll need to (setq inf-ruby-eval-binding "binding") in your config. But hopefully we can puzzle out the original goal of the change and do something compatible.

If I understand correctly Pry.toplevel_binding is used instead of binding to avoid evaluation inside of manually opened in REPL (and not yet closed) namespaces, pinning everything sent to REPL to scope of main object at all times. It's hard to comprehend how this is useful, considering that source code buffer is usually(?) a file, likely namespace itself, and evaluation results are displayed for the insides of such buffer under cursor. Implementation of output under cursor makes me feel like current scope/binding should be considered first, but it isn't 🤔

dgutov commented 7 months ago

If newly spawned inf-ruby-mode buffer will append itself automatically to inf-ruby-buffers and the commands will be sent to the latest appended we probably could check for result, and if receiver is non active then remove last buffer from the list and retry with previous item until nothing left in the list in case of all processes being terminated, like with LIFO stack. This option feels like an easy and simple one.

Ok, that could work. But it adds some extra management for the entries in the list.

Not sure that the last visited, instead of spawned, should be made active.

I suppose that for rspec buffers either would be fine. But outside of those, by visiting you could at least make the manual choice which one to use.

Anyway, see 37f4ec0 as the first approximation. It simply uses the first visible buffer that's in inf-ruby-mode.

dgutov commented 7 months ago

If I understand correctly Pry.toplevel_binding is used instead of binding to avoid evaluation inside of manually opened in REPL (and not yet closed) namespaces, pinning everything sent to REPL to scope of main object at all times. It's hard to comprehend how this is useful, considering that source code buffer is usually(?) a file, likely namespace itself, and evaluation results are displayed for the insides of such buffer under cursor.

Perhaps this is the idea?

[1] pry(main)> a = 123
=> 123
[2] pry(main)> Pry.toplevel_binding
=> #<Binding:0x00006428c0e6b430>
[3] pry(main)> Pry.toplevel_binding.local_variables
=> [:a, :__, :_, :_dir_, :_file_, :_ex_, :pry_instance, :_out_, :_in_]
[4] pry(main)> cd String
[5] pry(String):1> cd String#gsub
...
[8] pry(String):2> exit
=> String
[9] pry(String):1> a
NameError: undefined local variable or method `a' for String:Class
from (pry):7:in `__binding__'
[10] pry(String):1> exit
=> String
[11] pry(main)> a
=> 123
[12] pry(main)> Pry.toplevel_binding
=> #<Binding:0x00006428c0e6b430>
[13] pry(main)> Pry.toplevel_binding.eval('a')
=> 123

Like, you might jump around different contexts, but the expressions from the buffer will all evaluate in the same one, ensuring continuity. Seems nice to have, if maybe not essential. It would be great if we could discern one usage from another and choose the most appropriate binding at runtime.

x3qt commented 7 months ago

I think it may be possible to check if runtime is having a binding already that is not the top-level.

dgutov commented 7 months ago

Well, if it doesn't, binding would refer to the top-level binding (I think), so things would work to your preference already.

x3qt commented 7 months ago

I've checked in a few projects of mine and binding is not the same as Pry.toplevel_binding singleton even if it's launched from the project's root:

➜  api git:(master) be pry
[1] pry(main)> binding
=> #<Binding:0x00007f7f5da40180>
[2] pry(main)> Pry.toplevel_binding
=> #<Binding:0x00007f7f6e9e5b20>

One of the ideas I have is that probably compilation buffer may be using binding and "manual" one may be left with Pry.toplevel_binding for the sake of backwards compatibility.

Also, regarding latest changes as of c3049d5, I've noticed that Robe is not connecting to the compilation buffer, which is not very handy, otherwise it's almost perfect!

x3qt commented 7 months ago

Also I have noticed an issue with ruby-quit applied to compilation – on exit it somehow considers spec that should fail a success, which is very weird, considering that (process-send-string "*rspec-compilation*" "exit\r") behaves as expected, ie spec fails when there's a mismatch with expectation.

Screenshot 2024-05-07 at 18 35 52

UPD. Changing (ruby-send-string "exit") to (process-send-string (inf-ruby-proc) "exit\r") inside of ruby-quit seems to do the trick:

image
dgutov commented 6 months ago

I've noticed that Robe is not connecting to the compilation buffer, which is not very handy, otherwise it's almost perfect!

Thanks for noticing! ;-( (I understand you meant now, not not)

How to avoid that the best way...

dgutov commented 6 months ago

Try upgrading to both the latest inf-ruby and robe, that should be fixed now.

dgutov commented 6 months ago

Going back to the previous question...

I've checked in a few projects of mine and binding is not the same as Pry.toplevel_binding singleton even if it's launched from the project's root

It's a little weird: even if you just evaluate binding multiple times, you get different values/addresses.

And Pry.toplevel_binding apparently is a copy of TOPLEVEL_BINDING: https://github.com/pry/pry/blob/6f4d2c171efb7f80290db28204a2cf57a4b59736/lib/pry/pry_class.rb#L354-L369

Which has some purpose behind it (Pry evaluates different built-in features using that binding).

One of the ideas I have is that probably compilation buffer may be using binding and "manual" one may be left with Pry.toplevel_binding for the sake of backwards compatibility.

That could work.

Though if we hardcode that choosing logic, it will be more difficult for another user to switch it off, e.g. going back to the current behavior for compilation buffers as well. But OT2H it sounds closest to the optimal among the current choices.

x3qt commented 6 months ago

I've noticed that Robe is not connecting to the compilation buffer, which is not very handy, otherwise it's almost perfect!

Thanks for noticing! ;-( (I understand you meant now, not not)

How to avoid that the best way...

Try upgrading to both the latest inf-ruby and robe, that should be fixed now.

I meant not connecting, meaning that I wanted Robe to connect to newest (and only) REPL exposed by RSpec but it did not :)

I've also just got a chance to test latest versions of packages in question, in combination with (setq inf-ruby-eval-binding "binding") REPL commands work as expected.

Though if we hardcode that choosing logic, it will be more difficult for another user to switch it off, e.g. going back to the current behavior for compilation buffers as well. But OT2H it sounds closest to the optimal among the current choices.

I've tried implementing it myself but my Lisp-Fu is lacking required skill. Could you please create a patch for this?

dgutov commented 6 months ago

I meant not connecting, meaning that I wanted Robe to connect to newest (and only) REPL exposed by RSpec but it did not

Anyway, now it works, right?

I've tried implementing it myself but my Lisp-Fu is lacking required skill. Could you please create a patch for this?

Here's a diff to try out:

diff --git a/inf-ruby.el b/inf-ruby.el
index ab47519..8682032 100755
--- a/inf-ruby.el
+++ b/inf-ruby.el
@@ -706,11 +706,17 @@ Optionally provide FILE and LINE metadata to Ruby."
                                     (format ", %S" (file-local-name file)))
                                   (when (and file line)
                                     (format ", %d" line))))
+         (proc (inf-ruby-proc))
+         (inf-ruby-eval-binding (if (buffer-local-value
+                                     'inf-ruby-orig-compilation-mode
+                                     (process-buffer proc))
+                                    "binding"
+                                  inf-ruby-eval-binding))
          (code (format "eval(\"%s\", %s%s)\n"
                        (ruby-shell--encode-string string)
                        inf-ruby-eval-binding
                        file-and-lineno)))
-    (if (or (null (process-tty-name (inf-ruby-proc)))
+    (if (or (null (process-tty-name proc))
             (<= (string-bytes code)
                 (or (bound-and-true-p comint-max-line-length)
                     1024))) ;; For Emacs < 28
x3qt commented 6 months ago

I meant not connecting, meaning that I wanted Robe to connect to newest (and only) REPL exposed by RSpec but it did not

Anyway, now it works, right?

Unfortunately it is not :)

Here's a diff to try out:

Thanks a lot, now local binding is used indeed!

dgutov commented 6 months ago

Unfortunately it is not :)

Hmm... you wanted Robe to connect to the rspec repl? TBF it's not something I expected someone to want - because usually you would exit the repl soon enough, and the rspec process will end, and the Robe server would die with it.

Is that okay, or is that not what would be happening?

x3qt commented 6 months ago

Hmm... you wanted Robe to connect to the rspec repl? TBF it's not something I expected someone to want - because usually you would exit the repl soon enough, and the rspec process will end, and the Robe server would die with it.

Is that okay, or is that not what would be happening?

Well, that was the only REPL running and I felt that it may be natural if I go to definition without spinning another one, at least it worked this way with (setq inf-ruby-buffers (list (get-buffer "*rspec-compilation*"))) and was kinda handy, yet I understand that it may be outside of intended use :)

If we consider more real world scenario I have, usually there's already a spring server with application in test mode, which I start with robe to not wait seconds on rspec each time to load an app, so it's a REPL as well and is totally fine.

By the way, any news on that patch above? Can't wait to see it mainlined so me and folks could stop patching inf-ruby on releases 🚢 🙏

dgutov commented 6 months ago

Well, that was the only REPL running and I felt that it may be natural if I go to definition without spinning another one

Yeah, this is the reverse of the scenario I was considering: when you have some repl in the background, but the rspec one is the "first priority" - it would be a problem if it was picked for launching Robe, and then you shut it down soon.

If we consider more real world scenario I have, usually there's already a spring server with application in test mode, which I start with robe to not wait seconds on rspec each time to load an app, so it's a REPL as well and is totally fine.

Very good.

By the way, any news on that patch above? Can't wait to see it mainlined so me and folks could stop patching inf-ruby on releases 🚢 🙏

Okay, I'll go ahead and push it, let's see if anybody reports any problems later.

BTW, since you said "and folks" - I'm curious to hear about teams using Emacs for Ruby development. What works for you, and what had been the biggest pain points. If you have details to share, of course (private email might be best).

x3qt commented 6 months ago

Okay, I'll go ahead and push it, let's see if anybody reports any problems later.

Thanks a lot! Me and folks are really happy! 🎉

BTW, since you said "and folks" - I'm curious to hear about teams using Emacs for Ruby development. What works for you, and what had been the biggest pain points. If you have details to share, of course (private email might be best).

I don't know about any collaborative features of Emacs, except of Magit ofc which is really great. Mostly people I know who are using Emacs for Ruby development use it as any other editor (mostly space and doom distros), including me, ie a write a spec, run it, write implementation and iterate, but thanks to your patches now it enabled us to do live development without any context switches in almost automagical way and without additional configuration!

I had only one pain point, that I had to copy regions, switch buffer to compilation, paste etc, which is a lot of hurdle, but since now it's automated away experience is totally seamless!

dgutov commented 5 months ago

That's good to hear!

I don't know about any collaborative features of Emacs, except of Magit ofc which is really great. Mostly people I know who are using Emacs for Ruby development use it as any other editor (mostly space and doom distros)

I didn't mean collaborative features in particular, but rather it being comfortable for developers across a team, and for different setups. E.g. some do remote development, some use containers (different kinds), and that's not always frictionless in Emacs.

I do agree that our rspec workflow is pretty smooth, a secret weapon of sorts.

x3qt commented 5 months ago

That's good to hear!

I don't know about any collaborative features of Emacs, except of Magit ofc which is really great. Mostly people I know who are using Emacs for Ruby development use it as any other editor (mostly space and doom distros)

I didn't mean collaborative features in particular, but rather it being comfortable for developers across a team, and for different setups. E.g. some do remote development, some use containers (different kinds), and that's not always frictionless in Emacs.

I do agree that our rspec workflow is pretty smooth, a secret weapon of sorts.

I've heard from collegues that TRAMP is hard so never tried it myself, prefer to run things locally so can't comment on this 😀