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

Lazily load the multi-irb extension #472

Merged
merged 2 commits into from
Dec 7, 2022

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Dec 7, 2022

We now have plan to implement a command that prints all commands'
information, which will need to load all commands files without actually
running them.

But because the multi-irb extension patches IRB's top-level methods,
loading it would cause unintentional side-effects.

So this commit moves related requires into command execution to avoid the problem.

@st0012 st0012 mentioned this pull request Dec 7, 2022
lib/irb/cmd/subirb.rb Outdated Show resolved Hide resolved
lib/irb/cmd/chws.rb Outdated Show resolved Hide resolved
lib/irb/cmd/pushws.rb Outdated Show resolved Hide resolved
@st0012 st0012 force-pushed the lazily-load-command-extensions branch 2 times, most recently from d1af591 to c79147c Compare December 7, 2022 22:30
@st0012 st0012 changed the title Lazily load command extensions when possible Lazily load command extensions that has side-effects Dec 7, 2022
@st0012 st0012 force-pushed the lazily-load-command-extensions branch 2 times, most recently from 1503c81 to 3fc0fe8 Compare December 7, 2022 22:36
@st0012 st0012 changed the title Lazily load command extensions that has side-effects Lazily load the multi-irb extension Dec 7, 2022
We now have plan to implement a command that prints all commands'
information, which will need to load all command files without actually
running them.

But because the `multi-irb` extension patches IRB's top-level methods,
loading it would cause unintentional side-effects.

So this commit moves related requires into command execution to avoid the problem.
@st0012 st0012 force-pushed the lazily-load-command-extensions branch from 3fc0fe8 to b001cb4 Compare December 7, 2022 22:45
lib/irb/cmd/subirb.rb Show resolved Hide resolved
class MultiIRBCommand < Nop
def initialize(conf)
super
extend_irb_context
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is cleaner, wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

Copy link
Member

@k0kubun k0kubun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@k0kubun k0kubun merged commit f210b8a into ruby:master Dec 7, 2022
matzbot pushed a commit to ruby/ruby that referenced this pull request Dec 7, 2022
(ruby/irb#472)

* Lazily load the multi-irb extension

We now have plan to implement a command that prints all commands'
information, which will need to load all command files without actually
running them.

But because the `multi-irb` extension patches IRB's top-level methods,
loading it would cause unintentional side-effects.

So this commit moves related requires into command execution to avoid the problem.

* Make extend_irb_context private

Co-authored-by: Takashi Kokubun <takashikkbn@gmail.com>
@st0012 st0012 deleted the lazily-load-command-extensions branch December 7, 2022 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants