binding.pry doesn't fire when it's on the last line of a program about pry-byebug HOT 34 OPEN

Hi @JoshCheek, thanks for your feedback. I still feel the best resolution for this issue would be to make pry-byebug stop being a pry plugin and make it into an independent tool with a separate entry point other than binding.pry.

But I'm of course I'm open to smarter, better thought-out implementations of byebug that make it a better tool, regardless of this issue with pry-byebug.

Yeah, that makes sense. An alternative would be to leave it as a plugin, but one which you explicitly enter, eg a debug command to turn it on, and until then, it just delegates down to pry. Another option would be a different method, eg pry.debug so that the user could opt into the functionality of their preference.

Really, the pain I'm trying to solve is that one of my projects has it as a dependency, so it gets installed with that project, and I didn't really realize that. So, then when pry autoloads it, and it changes pry's behaviour, the line numbers are off, so I thought pry was buggy for ~6 months, and finally sat down last night to report it / try to fix it. Anything which addresses this unintentional loading and changing is sufficient, IMO.

Do note that I may be an exception as I often work without bundler, allowing this situation to occur.

from pry-byebug.

(╯°□°)╯︵ ┻━┻

from pry-byebug.

I've been recently considering to actually change the command to enter pry-byebug instead of reusing binding.pry. That way the user would be able to choose whether (s)he wants step-by-step debugging or just a plain REPL (like in this case, since step-by-step debugging doesn't make a lot of sense from the last line of a program).

Would that be a satisfactory resolution for this issue?

from pry-byebug.

FWIW, I got it to behave the way I would expect (didn't test much, just playing around):

diff --git a/lib/byebug/processors/pry_processor.rb b/lib/byebug/processors/pry_processor.rb
index 57a4ee1..2d3ae78 100644
--- a/lib/byebug/processors/pry_processor.rb
+++ b/lib/byebug/processors/pry_processor.rb
@@ -11,11 +11,23 @@ module Byebug
     def_delegators :@pry, :output
     def_delegators Pry::Helpers::Text, :bold

-    def self.start
+    def self.start(target=nil)
       Byebug.start
       Setting[:autolist] = false
-      Context.processor = self
-      Byebug.current_context.step_out(4, true)
+
+      # hack because byebug instantiates our class, so this lets us hang onto
+      # the binding until we have an instance to use it on
+      initializer = lambda do |*args, &b|
+        processor = new(*args, &b)
+        processor.enq_binding target if target.kind_of? ::Binding
+        processor
+      end
+      class << initializer
+        alias new call
+      end
+      Context.processor = initializer
+
+      Byebug.current_context.step_out(3, true)
     end

     #
@@ -37,6 +49,22 @@ module Byebug
       return_value
     end

+    def enq_binding(bnd)
+      enqueued_bindings << bnd
+    end
+
+    private def enqueued_bindings
+      @enqueued_bindings ||= []
+    end
+
+    private def new_binding
+      if enqueued_bindings.any?
+        enqueued_bindings.shift
+      else
+        frame._binding
+      end
+    end
+
     #
     # Set up a number of navigational commands to be performed by Byebug.
     #
@@ -106,8 +134,6 @@ module Byebug
     # Resume an existing Pry REPL at the paused point.
     #
     def resume_pry
-      new_binding = frame._binding
-
       run do
         if defined?(@pry) && @pry
           @pry.repl(new_binding)
diff --git a/lib/pry-byebug/pry_ext.rb b/lib/pry-byebug/pry_ext.rb
index abfdd96..583eefe 100644
--- a/lib/pry-byebug/pry_ext.rb
+++ b/lib/pry-byebug/pry_ext.rb
@@ -5,7 +5,7 @@ class << Pry

   def start_with_pry_byebug(target = TOPLEVEL_BINDING, options = {})
     if target.is_a?(Binding) && PryByebug.file_context?(target)
-      Byebug::PryProcessor.start unless ENV["DISABLE_PRY"]
+      Byebug::PryProcessor.start target unless ENV["DISABLE_PRY"]
     else
       # No need for the tracer unless we have a file context to step through
       start_without_pry_byebug(target, options)

After looking at it for a while and reading a lot of the byebug source, I finally realized that we mostly just needed to pass the supplied binding to pry on the first invocation, and then after that, it can behave the way it currently does.

It turned out to be a lot harder of a problem than I was would expecting, because byebug seems to mostly just be a wrapper around the tracepoint API. So all it can really do is hook into tracepoint events. Because we start pry-byebug after the call to binding.pry, there are no more events on that line.

Eg consider this program:

def a
  set_trace_func proc { |event, _file, lineno, methodname, binding, classname|
    desc = sprintf "%8s ", event
    desc << methodname.to_s if methodname
    printf "%-25s %2d: %s", desc, lineno, File.readlines(__FILE__)[lineno-1]
  }
end

def b
  123
end

a
b

When we run it, we will see the tracepoint events. It prints:

c-return set_trace_func    2:   set_trace_func proc { |event, _file, lineno, methodname, binding, classname|
  return a                 7: end
    line                  14: b
    call b                 9: def b
    line b                10:   123
  return b                11: end

So, we start on line 2, with a return from the C-function set_trace_func, then move to the end on line 7, where we return from a. If we then moved to line 13, the location we are returning to, this would be easy, but tracepoint skips that and goes to the next event: moving to line 14.

Frankly, it doesn't seem very thought-out. Eg replace the line that says b with b;b;b, since both the call and the return pass the method's binding, there doesn't seem to be a way to get access to the caller between the invocations. Whenever I've played w/ this API in the past, I always had to track everything and re-construct the missing events/data to present a coherent picture. I'm not sure why byebug doesn't do that.

Also, here's another example that I hit often, where the current implementation doesn't work (I use big case statements when iterating over things like ASTs):

object = 123

case object
when Integer
  puts 'its a number!'
  require "pry"
  binding.pry
when String
  puts 'its a string!'
when Regexp
  puts 'its a regex!'
end

from pry-byebug.

@squarism Thanks for the feedback. I have that vim snippet too :)

from pry-byebug.

Hi @squarism, thanks for the report. I'm aware of this issue, I had a similar report in byebug recently (deivid-rodriguez/byebug#98). I'll try to think of something to workaround this in the future, but it is not a priority...

from pry-byebug.

This is the same issue as #35. I didn't realize that this was still happening after 6e1c9a1 (I guess now it's only happening at the end of a script instead of the end of a block?). This is a common use case for Pry, so it would be nice to have it working.

from pry-byebug.

Rolled back to 1.3.2 only because of this. :(

from pry-byebug.

Hi @rf-! Yes, same issue, but only for end of script.

Since this issue was opened I've been thinking about what to do with pry-byebug. I was a byebug user who wanted some extra REPL functionality so I developed it to fullfil my own needs, but I didn't understand that most of the user base were actually the opposite: pry users wanting some stepping functionality. I should have probably been more careful about this. The current situation is that I hardly ever use pry-byebug, so I feel less motivated to mantain software I don't use. :(

In any case, I propose 2 alternatives:

• Make pry-byebug an "official" pry plugin (move it to pry/pry-byebug) and get someone from the pry community to mantain it.
• If nobody volunteers for 1, I'll eventually get to this, but I really don't know when that's going to be.

As I workaround while this is sorted out (for people who don't want to stick to the 1.x series), I suggest adding

gem `pry-byebug`, require: false

to Gemfile, and then doing

require 'pry-byebug'; binding.pry

normally but doing

require 'pry'; binding.pry

when at the last line of your program.

from pry-byebug.

Interesting. Just for the record, I found pry-byebug through pry-nav's README which recommended pry-byebug since pry-nav is (planned to be) abandoned. I didn't even know what byebug is! 😛

Thanks for the require tip, that's helpful for project style work although sometimes you don't have a Gemfile (scripts / bin / whatever). Not trying to dissuade you from your proposals while you are at the same time commenting that you are looking for motivation.

The workaround for the last line doesn't act as a workaround.

$ ruby -v
ruby 2.1.5p273 (2014-11-13 revision 48405) [x86_64-darwin13.0] 

$ cat test.rb
fruit = [
  { name: 'Apple', color: 'red', from_tree: true },
  { name: 'Orange', color: 'orange', from_tree: true },
  { name: 'Tomato', color: 'red', from_tree: false }
]
require 'pry'; binding.pry

$ gem list pry
pry (0.10.1)
pry-byebug (2.0.0)
pry-doc (0.6.0)

When running this, it exits. No pry shell.

$ ruby test.rb
$ echo $?
0

Interestingly, running test.rb with the pry command goes into pry's source code. Hmm.

pry test.rb

From: ~.rvm/gems/ruby-2.1.5/gems/pry-0.10.1/lib/pry/pry_instance.rb @ line 356 Pry#evaluate_ruby:

    351: def evaluate_ruby(code)
    352:   inject_sticky_locals!
    353:   exec_hook :before_eval, code, self
    354:
    355:   result = current_binding.eval(code, Pry.eval_path, Pry.current_line)
 => 356:   set_last_result(result, code)
    357: ensure
    358:   update_input_history(code)
    359:   exec_hook :after_eval, result, self
    360: end

But then it exits.

pry> code
=> "require 'pry'; binding.pry\n"
pry> Pry.eval_path
=> "(pry)"

Maybe I could compare states and dig into this to see the difference. Hmm.

Anyway:

1. Byebug by itself is a simple alternative which might work.
2. This gem is still solid gold. It gives ruby devs a killer IDE-like feature.
3. Debugging gems should probably all have require false in the Gemfile? In this case, pry loads plugins automatically anyway right?

I'm still trying to find the exact combination of gem versions that replicates the (off-topic) pry start up problem.

from pry-byebug.

@squarism Sorry for the false workaround, I didn't know pry would load plugins automatically, and just assumed that would work...

And thanks for your valuable comments! I think require: false for debugging gems is a somewhat good practice but not really that important. At least with byebug I've never noticed any significant difference in performance when booting my apps.

from pry-byebug.

@deivid-rodriguez Thanks for giving your perspective on the "byebug plus REPL" vs. "Pry plus stepping" thing. This is really the same problem as the one-letter aliases, I think. As a Pry user, I would normally expect a plugin to stay out of the way unless I'm specifically trying to use it. On the other hand, it makes perfect sense that someone coming from the byebug side would want to make their Pry experience as byebug-like as possible.

Anyway, I'm not sure anyone on the Pry team (including me) is going to want to commit to maintaining this project. Given that that's the case, and that you aren't using it, I wouldn't blame you at all for taking a while to fix this edge case.

from pry-byebug.

Re: the require: false workaround not working, you can put Pry.config.should_load_plugins = false in your .pryrc to disable automatic plugin loading.

from pry-byebug.

Yep, same problem as the aliases. Thanks for understanding, I'll way until someome wants to take over or until I find time myself. At least we have a valid workaround now!

from pry-byebug.

@deivid-rodriguez can you explain how to disable this behaviour? is it an easy change? If you can give me some guidance on reverting it back to the normal pry behaviour i'll take this on :)

from pry-byebug.

@banister It's been a while since I don't play with this code, but the change was introduced in the commit pointed out by @rf- in #35. The thing is I like the current behavior and I'd like to keep it and fix this edge case, but it's not an easy one...

from pry-byebug.

@deivid-rodriguez is it possible we could make this a config option and users could choose their behaviour? If possible, could we have teh default config to be the normal pry behaviour and for people who prefer your behaviour they could explicitly set it in their ~/.pryrc?

The unfortunate situation is that nearly everyone uses pry-byebug with pry (as it's an awesome gem, thanks! :), but it dramatically changes the standard/expected behaviour for most pry users. I would really really like it if we could have the old behaviour the standard.

Is this an acceptable compromise? I hope so!

from pry-byebug.

@deivid-rodriguez oh i just heard that you fixed it for the case where binding.pry is added on the last line of a method, that'll definiteily fix the 90% case.

Is it possible you could push out a new gem with this fix in it? I think that would reduce the majority of confusion, awesome 👍

John

from pry-byebug.

@banister I've been a bit reluctant to release because I haven't used last master myself in any app, and the current test suite doesn't give me enough confidence. Let me try the gem against an app in the next days and I'll release if everything is working fine. Or if you can confirm this yourself, I'll release right now! Does this sound reasonable?

from pry-byebug.

@banister done!

from pry-byebug.

The current situation is that I hardly ever use pry-byebug, so I feel less motivated to mantain software I don't use. :(

Are you able to add a note asking for a new maintainer at the top of the readme?

from pry-byebug.

@gerrywastaken Do you think that's going to work? I mean anybody interested could try to fix this issue (or any other), this is open source after all, but nobody has stepped in... I don't think this gem needs a new mantainer, it needs contributions!

from pry-byebug.

@deivid-rodriguez Oh sorry! It seems I misunderstood what you were saying. Sorry then, I take it back.

Regarding others fixing the issue. I suspect most want a revert of the auto advance feature, which would likely fix this and incompatibility issues with other pry gems. But I can understand reluctance to do this, given what you want from pry-byebug.

from pry-byebug.

No need to apologize! What I meant is that I currently have no motivation to fix some issues but I'm happy to review any patches and help contributors. If you think a note in the readme can encourage contrutions, it can be done.

I don't think this specific issue affects many people. And regarding pry-rescue compatibility, some may consider it a regression but I never meant to support it in the first place... it was just "luck".

from pry-byebug.

@deivid-rodriguez this auto-advance behaviour actually does break a bunch of plugins and causes weird/unexpected behaviour, i think it does affect a lot of people it's just they've either set the gem version of pry-byebug to a prior one that didn't have the auto advance or they've just had to work around the behaviour/live with it.

It would be awesome, if we could revert auto advance -- i think i asked this a while ago, but would you be willing to merge a PR that reverted this behaviour? I appreciate the idea behind the feature, but it just doesn't work well with the Pry ecosystem in general as it breaks expectations of other pry-related gems.

Again, I really appreciate the work you've done on this gem but this auto-advance behaviour is so out of tune with Pry in general that I'd really, really love to see it removed or at least turned off by default! :)

I'd be happy to do the work in reverting this behaviour if I know that you'd merge it :)

from pry-byebug.

@banister Please, could you reread this conversation? Last time you talked you requested this same thing, but you hadn't tried last master. Then someone pointed out to you that this was fixed in the 90% case. Then you said it was ok but you asked for a release. So I released and requested some feedback from you. But you didn't reply. Now you ask for the same thing but don't explain what has changed...

It's getting very difficult to me to understand what you want and why you want it. To the point that I feel you are disrespecting my time. Your input is always appreciated, you're 'pry`'s author after all, but I really think you're not adding any value.

from pry-byebug.

@deivid-rodriguez Sorry for not being clear :) Also, sorry for not responding earlier, i probably got distracted by some mundanity and forget to get back to you :(

Earlier in the discussion I was just considering pry-byebug as an isolated gem, the fix on master does generally fix pry-byebug as an isolated gem, but it has very bad effects (that i hadn't realized) for the pry ecosystem as a whole.

For example, auto advancing behaviour breaks 2 other very popular plugins: pry-rescue and pry-remote, i know that you're not explicitly supporting those, but people generally expect all the popular pry plugins to work in combination and become very confused when they don't.

I hadn't realized how confused people were by the auto advancing behaviour until relatively recently. I don't want to waste your time by asking you to remove the feature yourself, but would you consider merging a PR that reverted this?

:)

from pry-byebug.

@deivid-rodriguez Sorry for not being clear :) Also, sorry for not responding earlier, i probably got distracted by some mundanity and forget to get back to you :(

No problem, sorry if my comment sounded harsh.

I hadn't realized how confused people were by the auto advancing behaviour until relatively recently. I don't want to waste your time by asking you to remove the feature yourself, but would you consider merging a PR that reverted this?

No. I'll consider PR's focusing on adding a feature "X" that I want (I'm up for supporting the kind of functionality pry-rescue or pry-remote provide). If a specific PR, tests or reasoning over the code convinces me that adding feature "X" strictly requires removing feature "Y", then I'll consider it (I don't think this is the case for pry-remote, I'm sure it's not for pry-rescue). But PR's just focusing on reverting specific features..,. I don't want those.

Thanks anyways.

from pry-byebug.

@deivid-rodriguez I think this would be fine for me. I think most people wouldn't mind changing to a new thing. I personally have require "pry"; binding.pry as a vim snippet anyway. So, no problema.

from pry-byebug.

Hi @JoshCheek, thanks for your feedback. I still feel the best resolution for this issue would be to make pry-byebug stop being a pry plugin and make it into an independent tool with a separate entry point other than binding.pry.

But I'm of course I'm open to smarter, better thought-out implementations of byebug that make it a better tool, regardless of this issue with pry-byebug.

from pry-byebug.

I mean the "easiest". The best would be of course to fix this by somehow adding support for this in byebug. Or maybe making the autoadvance feature configurable, there's an old PR doing that that I never got to review.

from pry-byebug.

Any movement on this bug?

from pry-byebug.

Any movement on this bug?

from pry-byebug.

to have the binding.pry as the last line is useful for a console script, so it is important not to quit

from pry-byebug.