Thanks, man! That was fast.

I am a bit worried that Pry.start does not invoke Pry.final_session_setup now. Perhaps, you could invoke it right after initial_session_setup in Pry.start. But this would result in double invocation of Pry.final_session_setup, if you run Pry from shell, because you also call it from the cli.rb file.

I hope that makes sense.

A gentle ping.

Hrm. Sorry I missed your comment. I'm reading through now and trying to work out what's the best solution.

It's pretty hacky but I'm tempted to just attempt to make final_session_setup idempotent. I can mostly see what the refactor the option parsing needs would look like but unfortunately I don't have time.

Let me know what you think of these patches

Thanks!

I think the solution is close, but it won't work in some cases.

We invoke Pry.start in our specs a lot and with this change the session is not finalised anymore. Although the tests pass, it may lead us into wrong assumptions later. Also, any Pry plugin or other project that depends on Pry.start is affected.

So this kinda seems like an idiosyncrasy of ruby, in that it seems like invoking pry more than once in a single VM is probably going to be pretty dicey. should it reload all your plugins every time? Given that there's not any obvious way for plugins to deinitialize themselves it seems unlikely.

That said, I can add a shim to the tests that will just munge that variable back to nil between test runs, that would restore the status quo. Thoughts?

I forgot to mention that we actually use Pry.start for binding.pry as well :D

Anyway, what you said is a very good point. It's indeed tricky, that's why we use the initial_session? check to save us from unwanted reinitialisation. However, with the change presented in this PR the code below never invokes Pry.final_session_setup, which means that the session doesn't have any history anymore.

require 'pry'
Pry.start(self, {})

That said, I can add a shim to the tests that will just munge that variable back to nil between test runs, that would restore the status quo. Thoughts?

In my opinion the most optimal solution is to call final_session_setup right after initial_session_setup in Pry.start. The @session_finalized instance variable that you introduced saves us from the unwanted reinitialisation. I would also reset it.

Ok, I think this should be good to go! Thanks for your help sheparding!

Contributor of the week! 🎉