-
Notifications
You must be signed in to change notification settings - Fork 118
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
Support helper method registration #624
Conversation
@vinistock provided some feedback on this:
|
I like it 👍 Maybe there are some helper that should be defined only in main object. For example: irb(main):001> controller
=> <#ApplicationController:0x123>
irb(main):002> cws UsersController.new
irb(UsersController):003> controller
=> # If it returns ApplicationController instead of UsersController, it is misleading.
irb(main):001> cws Article.first
irb(Article):002> update author: controller.current_user
=> true
# User might think that it is possible to write the code above in model method, but it's not. If all of the commands/helpers are (This is just an idea for future, and I don't know if these kind of main-only helpers are really wanted.) |
@tompng That's a great point! But maybe we can improve it in another way? How about we let helpers access to the current main objects? That way the libraries can make that decision themselves. (I'd reserve those changes until we receive actual requests from library maintainers though) What I'd also mention is that if we make some commands strictly on the main object, it could be a breaking change to some users. |
Out of curiosity, why the use of |
@bkuhlmann That's a great point. I picked
Could you elaborate on this? |
Stan: Yeah, so I was thinking this would be neat in terms of closures: # Proc
demo = proc { |*positionals, **keywords, &block| "TOOD: Implementation details." }
IRB::Helper.register :demo, demo
# Lamda
demo = -> *positionals, **keywords, &block { "TOOD: Implementation details." }
IRB::Helper.register :demo, demo Then, once the closure is registered, you could send the I'm making a couple of assumptions, though, which I don't believe matches up with your current implementation:
Anyway, some thoughts. Given the value of inheriting from the |
@bkuhlmann Thanks for the explanation 👍 Yeah under the current plan just passing a Proc in wouldn't work because of But as you said, not having description would be a huge disadvantage. Since one of the main goals here is to make helpers more discoverable through these extra metadata, supporting this feel like a step backward to me. I don't plan to support it right away, but I'm open to other people's feedback on this idea. And thank you so much for taking the time to propose it 🙌 |
fe4ee5d
to
fa5b682
Compare
fa5b682
to
a2a92c5
Compare
60eb021
to
608f07f
Compare
608f07f
to
f836546
Compare
@tompng I think this can go hand in hand with the command extension API. WDYT? |
lib/irb/default_commands.rb
Outdated
def self.install_helper_methods | ||
HelperMethod.helper_methods.each do |name, helper_method_class| | ||
define_method name do |*args, **opts, &block| | ||
verbose, $VERBOSE = $VERBOSE, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kind of warning need to be suppressed here?
Shouldn't we show a warning occurred inside helper.execute
implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to memoize the helper method instance, but then in Ruby 2.7 it'd cause the ivar not initialized warning.
lib/irb/default_commands.rb
Outdated
instance_variable_set(helper_ivar, helper) | ||
end | ||
|
||
helper.execute(*args, **opts, &block) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Helper method is an instance method of workspace.main
. If we pass self
to execute, perhaps helper_method can do more.
helper.execute(self, *args, **opts, &block)
We can get a similar value by IRB.CurrentContext.workspace.main
except for this case: $a=self
chws ""
$a.conf
(main==""
but self==$a
), so we don't need to pass it?
What do you think?
lib/irb/default_commands.rb
Outdated
|
||
unless helper | ||
helper = helper_method_class.new | ||
instance_variable_set(helper_ivar, helper) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Command execution uses new(irb_context).execute(arg)
and does not cache the instance.
I think HelperMethod don't need to cache too, or should cache in another place to avoid adding instance variables to workspace.main
From: a.rb @ line 2 :
1: class A
=> 2: def f = binding.irb
3: end
irb(#<A:0x000000013019fb00>):001> conf;context;irb_context;
irb(#<A:0x000000013019fb00>):002> self
=>
#<A:0x000000013019fb00
@_helper_method_conf=#<IRB::HelperMethod::Conf:0x0000000127985878>,
@_helper_method_context=#<IRB::HelperMethod::Context:0x0000000127985670>,
@_helper_method_irb_context=#<IRB::HelperMethod::IrbContext:0x00000001279854b8>>
Can we leave I think helper method have a large benefit, and also have problem of polluting object with chws and debug. IRB uses SimpleDelegator for frozen object, so Other thoughtsRails's
|
I think at least
I feel we're talking about 2 related, but not identical problems. While polluting the current context is not ideal, it's already been done in contexts like Rails console to provide helper features for years. And another problem is that those features are hard to discover. Through this PR, my main goal is to only solve the feature discovery problem because this was a feedback from Rails maintainers. And this type of recurring Rails tips also demonstrates how beneficial it'd be if we can improve it. At the same time though, I never heard about people complaining about context pollution, so to me it could be solved much later. And I believe that by introducing official registration APIs first, it'll also make solving the pollution much easier. |
Ok, I think we can implement it as a helper_method now and make it show a deprecation message later. Let's go ahead! |
29f8ed7
to
33e0d92
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
Summary
This Pull Request introduces a helper extension API as discussed in issue #588. It aims to provide an API for helper methods in IRB, improving the method-extending approach currently in use. The new approach would make helper methods more discoverable and user-friendly.
Background
There are two categories of operations in IRB: Helper methods and Commands.
Helper methods
app
for a Rails application instance.ExtendCommandBundle
, likecontroller
orapp
in Rails console.Commands
show_source Foo#bar
.edit
) or display information (likehelp
). The return value is generally not important.Motivation
The current method-extending approach has several limitations:
ExtendCommandBundle
is an internal component that is currently exposed, which is not ideal.Implementation
This PR introduces a new
IRB::HelperMethod::Base
class that providesdescription
methods to helper classes. After requiringirb/helper_method
, users can useIRB::HelperMethod.register(:my_helper, MyHelper)
to add new helpers. Once registered, amy_helper
method will be added to IRB sessions.Here's an example:
Changes
A new
IRB::HelperMethod::Base
class has been added.Users can now register new helpers using
IRB::HelperMethod.register(:my_helper, MyHelperMethod)
.conf
is now declared as helpers.help
now also displays registered helpers.