Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove cli.rb to prevent loading of plugin #98

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mikz
Copy link

@mikz mikz commented May 24, 2016

pry will load all cli.rb files from plugins, no matter when they are disabled or not

and loading cli.rb will load all extensions and fully loading the plugin
so Pry.plugins['byebug'].disable! does not work
neither does Pry.config.should_load_plugins = false

removing cli.rb fixes it
I need to disable pry-byebug for some cases because of #69.

looks like it was introduced (https://github.com/deivid-rodriguez/pry-byebug/commits/77e1c046f89ae4b4dc255e6c5867415a20a78cec/lib/pry-debugger/cli.rb) to support pry-remote but that does not work much (#33)

Other pry plugins using cli.rb actually do something with the Pry::CLI which is not the case here. See https://github.com/pry/pry-exception_explorer/blob/540d7c9d139d6493083c3dc5c18315c1db100cc1/lib/pry-exception_explorer/cli.rb for an example.

pry will load all cli.rb files from plugins, no matter when they are disabled or not

and loading cli.rb will load all extensions and fully loading the plugin
so `Pry.plugins['byebug'].disable!` does not work
neither does `Pry.config.should_load_plugins = false`

removing cli.rb fixes it
@deivid-rodriguez
Copy link
Owner

Hi @mikz, thanks for this.

I think we need some more research to get this merged. Please refer to this comment for context and let me know!

@mikz
Copy link
Author

mikz commented May 24, 2016

@deivid-rodriguez I see. So you need pry-byebug to monkeypatch Pry.start and the only place to do that is cli.rb ? Because everything else is loaded afterwards ? Uf.

Would any of the Pry hooks be helpful there?

@deivid-rodriguez
Copy link
Owner

I honestly don't know why this is needed, but removing it seems to cause a regression..., at least it did in the past 😞

So I prefer to leave it for now unless someone can make sure we don't regress.

@mikz
Copy link
Author

mikz commented May 24, 2016

I can make sure that it does not regress. I just need to know why it tries to monkeypatch Pry.start. Would be enough just initializing the Byebug::PryProcessor when the plugin is initialized in https://github.com/pry/pry/blob/f5c97cac103a9f04b854ed5b117a8cc27c7e8dd6/lib/pry/pry_class.rb#L177 ?

@deivid-rodriguez
Copy link
Owner

@mikz If you can get pry-byebug working without monkeypatching Pry.start, I think that's a further improvement besides this PR! 👍 I don't remember any technical reason why monkeypatching would be compulsory. It's just the way it's always been implemented but I'm up for changing it.

@mikz
Copy link
Author

mikz commented May 24, 2016

Ok. To drop the cli.rb I'd need to get rid of the monkeypatch too. From what I see it should be just fine.

I'll try this to verify the regression:

echo "require 'pry'; binding.pry; puts 'next statement'" > foo.rb
ruby foo.rb
# next

@deivid-rodriguez
Copy link
Owner

Ideally we should add a regression test for posterity. But if you don't have time I guess we could live without it.

@deivid-rodriguez
Copy link
Owner

@mikz Ping. Did you forget about this or just decided not to work on it?

@mikz
Copy link
Author

mikz commented Dec 8, 2016

@deivid-rodriguez kind of both. IIRC there was more places to fix and I just did not have enough time.

I'd like to fix this in the future, just not sure when I'll find time.

@deivid-rodriguez
Copy link
Owner

Sounds good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants