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 autoloading of plugins #2119

Merged
merged 4 commits into from Nov 28, 2020

Conversation

mvz
Copy link
Contributor

@mvz mvz commented Mar 22, 2020

Fixes #1241.

  • Deprecate command line options that have to do with plugins.
  • Remove plugin system. Plugins can be loaded through regular require.

I've tested this with pry-doc and pry-byebug, and using require from .pryrc loads those just fine.

@kyrylo
Copy link
Member

kyrylo commented Nov 28, 2020

This looks good and I want to express my +1 for the effort. There's a problem with backwards compatibility, though. Pry loads cli.rb and we can add custom options. Example: https://github.com/pry/pry-exception_explorer/blob/master/lib/pry-exception_explorer/cli.rb

I described this in #1241, but I want to repeat this again. This is a breaking change but I think it's okay. There are no popular gems that define custom flags on the Pry executable.

@kyrylo kyrylo merged commit a619cfb into pry:master Nov 28, 2020
kyrylo added a commit that referenced this pull request Nov 28, 2020
@mvz mvz deleted the issue-1241-remove-autoloading-of-plugins branch November 28, 2020 17:16
@bf4
Copy link

bf4 commented Feb 9, 2021

re: deivid-rodriguez/pry-byebug#343 seems there's a missing open/closed issue in this repo saying 'maybe don't just upgrade yet?' :) I didn't see one and since I haven't delved into the issue thought I'd just poke here :)

I always thought the Pry plugin loading system was particularly interesting. One option would be to adjust the breaking change to make it opt-in or have have a 'debug' mode which uses it to find available plugins. I'd be willing to make such a PR if you want. (I tried to fix the readline issues a while ago so I've been in the repo before)

@mvz
Copy link
Contributor Author

mvz commented Feb 9, 2021

Probably an opt-in for this change with a deprecation warning would be good. I really hope it's possible to modify pry-byebug so it doesn't need to modify Pry's internals so much.

@bf4
Copy link

bf4 commented Feb 16, 2021

I made a PR, basically git diff 69019763876ef45f2e034f5023c7f5e5cb195995..714bcb5b3abd79587839d070ab6b58ff87a69399 | git apply then noodled the defaults a bit #2177 Didn't run the test suite, but figure it's good for review of intent

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.

Remove autoloading of Pry plugins
3 participants