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

Support helper method registration #624

Merged
merged 1 commit into from
Apr 23, 2024
Merged

Support helper method registration #624

merged 1 commit into from
Apr 23, 2024

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Jul 3, 2023

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

  • They are designed to be used as Ruby methods.
  • Users primarily use them for their return values, such as calling app for a Rails application instance.
  • At present, this is achieved by directly extending ExtendCommandBundle, like controller or app in Rails console.

Commands

  • They are intended to be used with their own syntax, for example, show_source Foo#bar.
  • Users utilize them to complete certain tasks (like edit) or display information (like help). The return value is generally not important.
  • They will also have an official registration API, as discussed in Support command extension APIs #513.

Motivation

The current method-extending approach has several limitations:

  • ExtendCommandBundle is an internal component that is currently exposed, which is not ideal.
  • IRB cannot effectively detect and aid users in discovering the extended helpers.
  • Due to the above reasons, we don't officially support or mention a method to extend IRB with helpers, which has likely limited IRB's potential for personal or library customization.

Implementation

This PR introduces a new IRB::HelperMethod::Base class that provides description methods to helper classes. After requiring irb/helper_method, users can use IRB::HelperMethod.register(:my_helper, MyHelper) to add new helpers. Once registered, a my_helper method will be added to IRB sessions.

Here's an example:

# test.rb

# this doesn't require the entire IRB, just the helper registration API
require "irb/helper_method"

class MyHelperMethod < IRB::HelperMethod::Base
  description "Describe how to use this helper"

  # all kinds of arguments are supported
  def execute(*args, **opts, &block)
    "Hello from MyHelperMethod#execute"
  end
end

IRB::HelperMethod.register(:my_helper, MyHelperMethod)

binding.irb

# irb(main):001:0> puts my_helper
# Hello from MyHelperMethod#execute
# => nil

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.

    IRB
      exit           Exit the current irb session.
      irb_load       Load a Ruby file.
      irb_require    Require a Ruby file.
      source         Loads a given file in the current session.
      irb_info       Show information about IRB.
      show_cmds      List all available commands and their description.
      history        Shows the input history. `-g [query]` or `-G [query]` allows you to filter the output.
    
    Workspace
      cwws           Show the current workspace.
      chws           Change the current workspace to an object.
      workspaces     Show workspaces.
      pushws         Push an object to the workspace stack.
      popws          Pop a workspace from the workspace stack.
    
    Multi-irb (DEPRECATED)
      irb            Start a child IRB.
      jobs           List of current sessions.
      fg             Switches to the session of the given number.
      kill           Kills the session with the given number.
    
    Debugging
      debug          Start the debugger of debug.gem.
      break          Start the debugger of debug.gem and run its `break` command.
      catch          Start the debugger of debug.gem and run its `catch` command.
      next           Start the debugger of debug.gem and run its `next` command.
      delete         Start the debugger of debug.gem and run its `delete` command.
      step           Start the debugger of debug.gem and run its `step` command.
      continue       Start the debugger of debug.gem and run its `continue` command.
      finish         Start the debugger of debug.gem and run its `finish` command.
      backtrace      Start the debugger of debug.gem and run its `backtrace` command.
      info           Start the debugger of debug.gem and run its `info` command.
    
    Misc
      edit           Open a file with the editor command defined with `ENV["VISUAL"]` or `ENV["EDITOR"]`.
      measure        `measure` enables the mode to measure processing time. `measure :off` disables it.
    
    Context
      help           [DEPRECATED] Enter the mode to look up RI documents.
      show_doc       Enter the mode to look up RI documents.
      ls             Show methods, constants, and variables. `-g [query]` or `-G [query]` allows you to filter out the output.
      show_source    Show the source code of a given method or constant.
      whereami       Show the source code around binding.irb again.
    
    Helper methods
      conf           Returns the current context.
    
    Aliases
      $              Alias for `show_source`
      @              Alias for `whereami`
    

@st0012 st0012 added enhancement New feature or request Don't merge labels Jul 3, 2023
@st0012 st0012 self-assigned this Jul 3, 2023
lib/irb/helper.rb Outdated Show resolved Hide resolved
lib/irb/cmd/show_cmds.rb Outdated Show resolved Hide resolved
lib/irb/helper/measure.rb Outdated Show resolved Hide resolved
@st0012 st0012 requested a review from a team July 4, 2023 11:10
@st0012
Copy link
Member Author

st0012 commented Jul 4, 2023

@vinistock provided some feedback on this:

  • To end users, the distinction between helpers and commands are not very useful. So a separate entry in show_cmds may be confusing instead.
  • The name Helper doesn't explain the concept very well.
    • Maybe HelperMethod is better?

lib/irb/cmd/show_cmds.rb Outdated Show resolved Hide resolved
@tompng
Copy link
Member

tompng commented Jul 5, 2023

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 commands or helpers only for main, we could completely avoid helper method polluting workspace object, especially in debug that current workspace object will change frequently by step.

(This is just an idea for future, and I don't know if these kind of main-only helpers are really wanted.)

@st0012
Copy link
Member Author

st0012 commented Jul 6, 2023

@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.

@bkuhlmann
Copy link

Out of curiosity, why the use of IRB::Helper::Base#execute? Could IRB::Helper::Base#call be the Object API instead? This would allow anyone to use the command pattern which would give folks more power to register simple closures if desired. 🚀

@st0012
Copy link
Member Author

st0012 commented Jul 10, 2023

@bkuhlmann That's a great point. I picked execute because it's the current API on (still private) command objects, but I don't feel strongly about it.

would give folks more power to register simple closures if desired

Could you elaborate on this?

@bkuhlmann
Copy link

bkuhlmann commented Jul 10, 2023

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 #call message to the registered helper. This could be quite handy for simple helpers. Anything more than that would require using a class which implements the #call message. Regardless, any implementation would be interchangeable as long as it adhered to the #call message.

I'm making a couple of assumptions, though, which I don't believe matches up with your current implementation:

  • Each helper, once registered, would only need to receive the #call message. Currently, it looks like you want to send the .new message to each helper and then, later, send the #execute message.
  • The major disadvantage, because a closure wouldn't inherit from Base, would mean there would be no description or context. That could be bad but...could work like Rake where if missing a desc shows up as hidden. Probably not desirable but food for thought.

Anyway, some thoughts. Given the value of inheriting from the Base class, the above might not be desirable. ...but using the #call message instead of #execute might still be of interest.

@st0012
Copy link
Member Author

st0012 commented Jul 11, 2023

@bkuhlmann Thanks for the explanation 👍

Yeah under the current plan just passing a Proc in wouldn't work because of .new. We can hide Helper.new.call under Helper::Base.call to make it work though.

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 🙌

@st0012 st0012 force-pushed the implement-#588 branch 2 times, most recently from fe4ee5d to fa5b682 Compare August 9, 2023 12:31
@st0012 st0012 changed the title Support helper registration Support helper method registration Aug 9, 2023
@st0012 st0012 force-pushed the implement-#588 branch 4 times, most recently from 60eb021 to 608f07f Compare February 1, 2024 22:21
@st0012
Copy link
Member Author

st0012 commented Apr 15, 2024

@tompng I think this can go hand in hand with the command extension API. WDYT?

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
Copy link
Member

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?

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 want to memoize the helper method instance, but then in Ruby 2.7 it'd cause the ivar not initialized warning.

instance_variable_set(helper_ivar, helper)
end

helper.execute(*args, **opts, &block)
Copy link
Member

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?


unless helper
helper = helper_method_class.new
instance_variable_set(helper_ivar, helper)
Copy link
Member

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>>

@tompng
Copy link
Member

tompng commented Apr 16, 2024

Can we leave context and conf as a command and have no helper method by default?
We can add IRB.context as an alias of IRB.CurrentContext and use it instead of these helper methods.

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 chws 1 [self, 1].uniq returns [1, 1].
For this reason, I'd like to make default IRB clean, and have a stance that no-helper is recommended. User can compare pros and cons, and decide adding helper method or not.

Other thoughts

Rails's reload! can be a command. app helper controller new_session does not need to be used from model(chws User.first). It can be a method of main object. Perhaps adding to Kernel is better than polluting by helper method. Taking this into account, one possible steps of upgrading commands/helpers might be:

  1. Add helper_method API without default helper methods
  2. Add a way to show help message for method defined in Kernel or main object
  3. Rails can migrate some helper_method to main method with help message added in step 2. Other apps can still use helper_method.

@st0012
Copy link
Member Author

st0012 commented Apr 16, 2024

Can we leave context and conf as a command and have no helper method by default?

I think at least conf has to be a helper method as the current documents all reference to it for changing configs inside the session (example)? And commands should NOT return values (or just nil) so they're not suitable for this usage.

I think helper method have a large benefit, and also have problem of polluting object with chws and debug.

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.

@tompng
Copy link
Member

tompng commented Apr 17, 2024

at least conf has to be a helper method

Ok, I think we can implement it as a helper_method now and make it show a deprecation message later. Let's go ahead!

@st0012 st0012 force-pushed the implement-#588 branch 2 times, most recently from 29f8ed7 to 33e0d92 Compare April 22, 2024 20:54
Copy link
Member

@tompng tompng left a 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 👍

@tompng tompng merged commit f9347b1 into master Apr 23, 2024
59 checks passed
@tompng tompng deleted the implement-#588 branch April 23, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

4 participants