Step #489

Closed
trans opened this Issue Mar 11, 2012 · 14 comments

Projects

None yet

5 participants

@trans
trans commented Mar 11, 2012

Please add support for code stepping from binding.pry point.

pry_debug is no fun, pry-nav bombs.

@banister
Member

in what way does pry-nav bomb? could you file an issue on pry-nav

@trans
trans commented Mar 12, 2012

Yep, already did. nixme/pry-nav#9

@banister
Member

Can you add a require "pry-nav" before the binding.pry call?

@banister
Member

Let me know if that fixes it

@trans
trans commented Mar 12, 2012

Now, don't I feel like a lump. It works perfectly.

Well, in my defence, I was under the impression that Pry had a plugin system that loaded such things automatically. So okay. Still, would be true awesomeness if pry-nav was built-in.

Thanks!

@trans trans closed this Mar 12, 2012
@trans trans reopened this Mar 12, 2012
@ConradIrwin
Member

You're right, it should have automatically loaded that, are you using Bundler?

@banister
Member

@ConradIrwin, @trans I think the issue is that the current Pry gem postpones plugin loading until the last-minute; which is too late for pry-nav (though fine for 99% of other plugins).

As far as i understand it, pry-nav works by monkey-patching the Pry.start method - but this monkey-patch can't take effect as it's only applied after the Pry.start method is already invoked.

A solution to this problem is just to load plugins much earlier, which i intend to do in the next release. This will allow plugins that operate via monkey-patching Pry.start to work correctly without needing a separate require

Basically this problem exists because i never foresaw plugins monkey-patching Pry.start :)

@envygeeks
Contributor

Or we could do a proper evaluation of my plugin system and how it fixes this problem already at it's core. How do you know you won't break those 99% of plugins if you load much earlier too? This sounds like a dangerous change that is only taking into account pry-nav which shouldn't trump all the other plugins on it's own. There is no real proper thought behind addressing this issue other then adjusting the load time which could just break other plugins.

@banister
Member

@envygeeks If moving plugin loading to an earlier stage breaks other plugins then it's a bug in the plugin. I only made guarantees about the relative order of loading (i.e .pryrc -> plugins -> history) and no guarantees about when exactly a plugin will be loaded.

Furthermore i cannot see how it could possibly break any other plugins. In fact, im 100% certain nothing will be broken by such a change. :)

@envygeeks
Contributor

@banister So wait, let me get this right. If @nixme plugin breaks it's a bug in Pry, but if another plugin breaks it must be a bug in the plugin? I don't understand that selective logic at all. How can you so boldly state that there will be no problems with any plugin when you couldn't even guarantee there wouldn't be problems with the current state? What happens if a plugin needs to monkey patch something that comes AFTER you load it? They are screwed and you've modified the state of all plugins for a single plugin rather then redesigning the system to think about both situations you've just procrastinated the problem until it's experienced by another author.

@banister
Member

@envygeeks the pry-nav plugin not working was not really a bug so much as an oversight -- i never foresaw people monkey-patching Pry.start. However if plugin loading is moved back prior to Pry.start then monkey-patches will work and other plugins should be unaffected :)

@envygeeks
Contributor

@banister Fair enough. Though I still think we need to think about a dual phase plugin system that actually cares about the state of it's plugins (like the one I designed) that doesn't force a single state on the author but allows at least 2 states and has the ability to even hook [after I push up my new hook and command helpers] where they want (a lot easier then they can now, they can obviously hook now but not many authors understand the API, these helpers try to lessen that barrier.)

@nixme
nixme commented Mar 16, 2012

@banister Is there anything I should change on the pry-nav side?

@banister
Member

@nixme nothing yet, i'll let you know though. Could you add a note to the pry-nav readme indicating they have to require 'pry-nav' explicitly? cheers :)

@banister banister closed this Apr 14, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment