Move away from method-based command implementation #824

tompng · 2023-12-24T16:49:35Z

Command implementation without using obj.extend IRB::ExtendCommandBundle. Command is not a method anymore.
Fixes #592
Continuation of #547

Command command arg was executed by eval("#{command} #{arg}") or eval("command #{transformed_args(arg)}").
This pull request will change command execution to load_command(command).execute(arg).

No main object method polluting by default

Now, IRB has no helper method by default.
~~If more than one helper method is defined in ExtendCommandBundle, (like Rails console adds app new_session reload! helper controller), IRB will reluctantly pollute main object.~~ (postponed)

What kind of input is a command?

IRB has command override policy NO_OVERRIDE OVERRIDE_PRIVATE_ONLY and OVERRIDE_ALL for defined methods.
New command recognition is:

Every command is single line, so multiline input is not a command
command_name arg might be a command
check private_method and public_method with the same name exists or not, and follow the override policy.

input	situation	type(before)	reason(before)	type(after)	reason(after)
`exit`	private method `Kernel.exit` is defined	command	alias `irb_exit` to `exit` because it's ORVERRIDE_PRIVATE_ONLY	command	command name exists and it's OVERRIDE_PRIVATE_ONLY
`exit`	public method `exit` defined by user	expression	does not alias `irb_exit` to `exit`	expression	command name exists but it's OVERRIDE_PRIVATE_ONLY
`irb_measure`		command	ruby evaluates as method because command method exists	command	command name exists
`measure`	local var `measure` defined	expression	ruby evaluates as lvar	command	command name exists
`measure off`	local var `measure` defined	command	ruby evaluates as method call	command	command name exists
`info 'a' + 'b'`	context.main is Logger.new	wrong expression	`logger.info("'a' + 'b'")` because IRB transform args even if command is not installed by NO_OVERRIDE policy	expression	`logger.info('a' + 'b')` because info is NO_OVERRIDE
`show_source * -s`	local var `show_source` defined	command	`show_source("* -s")` because transorm_args is defined	command	command name exists
`show_source += 1`		command	`show_source("+= 1")` because transform_args is defined	command	command name exists
`info += 1`		expression	ruby evaluates as opassign	command	command name exists

Command completion

Command was a method, so completor can complete it without extra effort.
Now, command is not a method, so this pull request adds command name completion to both RegexpCompletor and TypeCompletor.

`conf` and `context` method

conf context method can be used as conf.eval_history = 100 before.
It is now a command only to show configuration.

The source code comment says it just displays the configuration, and navigates to use IRB.conf for modifying it.

# Displays current configuration.
#
# Modifying the configuration is achieved by sending a message to IRB.conf.
def irb_context
  IRB.CurrentContext
end

@ALIASES = [
  [:context, :irb_context, NO_OVERRIDE],
  [:conf, :irb_context, NO_OVERRIDE],
  ...
]

But a different thing is written in the document that you can use conf.eval_history = N.

We can:

Fix the document (My proposal)
Show a message when input is /\A(conf|context)\./v
Add a special path to allow it, e.g. context.evaluate(code.gsub(/\A(conf|context)\./, 'IRB.CurrentContext.'))
Keep conf and context as a method. Polluting main object problem remains.

lib/irb/command/base.rb

lib/irb/command/help.rb

lib/irb/completion.rb

st0012 · 2024-04-01T11:35:31Z

lib/irb/statement.rb

-      def evaluable_code
-        @code
+      def execute(context, line_no)
+        context.evaluate(@code, line_no)


IMO, statement objects should only provide information and the evaluation should still happen in the Irb objects.

Then we need to introduce this kind of branch, because we can't provide evaluable_code for command.

case statement when Statement::Expression context.evaluate(statement.code, line_no) when Statement::Command ret = statement.command_class.execute(context, statement.arg) context.set_last_value(ret) end

I think def execute is better than this case-when.

I still prefer having the case statement inside Irb#eval_input than making statements perform the evaluation. Otherwise we're risking having a wrong coupling like RubyLex had, which is hard to eradicate as we both know.

Also, in the future (maybe even in the same release), we'll change commands to NOT have a return value. When that happens, it'd be even less beneficial to encapsulate things in statements.

st0012 · 2024-04-03T19:08:06Z

lib/irb/command/base.rb

+        # Use throw and catch to handle arg that includes `;`
+        # For example: "1, kw: (2; 3); 4" will be parsed to [[1], { kw: 3 }]
+        catch(:EXTRACT_RUBY_ARGS) do
+          @irb_context.workspace.binding.eval "IRB::ExtendCommand.extract_ruby_args #{arg}"


Let's use the new class:

Suggested change

@irb_context.workspace.binding.eval "IRB::ExtendCommand.extract_ruby_args #{arg}"

@irb_context.workspace.binding.eval "IRB::Command.extract_ruby_args #{arg}"

st0012 · 2024-04-03T19:22:20Z

lib/irb/statement.rb

-      def evaluable_code
-        @code
+      def execute(context, line_no)
+        context.evaluate(@code, line_no)


I still prefer having the case statement inside Irb#eval_input than making statements perform the evaluation. Otherwise we're risking having a wrong coupling like RubyLex had, which is hard to eradicate as we both know.

Also, in the future (maybe even in the same release), we'll change commands to NOT have a return value. When that happens, it'd be even less beneficial to encapsulate things in statements.

st0012 · 2024-04-03T19:37:13Z

lib/irb/workspace.rb

-    def load_commands_to_main
-      main.extend ExtendCommandBundle
+    def load_helper_methods_to_main
+      if ExtendCommandBundle.has_helper_method? && !(class<<main;ancestors;end).include?(ExtendCommandBundle)


I think it's fine to extend the context even if ExtendCommandBundle has no methods as it'll simplify tests (e.g. we don't need 2 cases to test workspace commands). We'll also come up with a better solution for this when we support helper methods.

Done!
I think it's beneficial to keep main object clean as possible, but I'll consider it later after helper method support

test/irb/test_type_completor.rb

st0012 · 2024-04-03T19:53:05Z

lib/irb.rb

+      end
+
+      # Check visibility
+      local_variable = @context.local_variables.include?(command)


While it's considerate to take local variables into account, I feel local variables should never override a command's invocation because:

It's the behaviour of current IRB, Pry, and debug, so I'd expect most users to be aware of this limitation already.

Under the current behaviour, users can still use p <var> to retrieve the variable when a conflict happens.

However, under the this version users will NOT be able to call a command if a same-name local is present.

I think the limitation is fine, but if we're going to fix #803, to be consistent, I think local variables should take precedence over methods.

info = 1 # assignment is considered not a command in this pull request => 1 info + 1 # this will be a command call if we don't consider local variables => 2

Yeah then it's probably better to leave the behaviour in #803 as is. I'll close it if you also agree.

Although there's many things to consider, I want to keep #803 open because it will be an enhancement.

lib/irb/command.rb

lib/irb/command/base.rb

lib/irb/command/help.rb

lib/irb/command/ls.rb

lib/irb/statement.rb

test/irb/test_command.rb

st0012

👍

lib/irb/command/help.rb

Co-authored-by: Stan Lo <[email protected]>

(ruby/irb#824) * Command is not a method * Fix command test * Implement non-method command name completion * Add test for ExtendCommandBundle.def_extend_command * Add helper method install test * Remove spaces in command input parse * Remove command arg unquote in help command * Simplify Statement and handle execution in IRB::Irb * Tweak require, const name * Always install CommandBundle module to main object * Remove considering local variable in command or expression check * Remove unused method, tweak * Remove outdated comment for help command arg Co-authored-by: Stan Lo <[email protected]> --------- ruby/irb@8fb776e379 Co-authored-by: Stan Lo <[email protected]>

This has a few benefits: - We can keep hiding the evaluation logic inside the Context level, which has always been the convention until #824 was merged recently. - Although not an official API, gems like `debug` and `mission_control-jobs` patch `Context#evaluate` to wrap their own logic around it. This implicit contract was broken after #824, and this change restores it. In addition to the refactor, I also converted some context-level evaluation tests into integration tests, which are more robust and easier to maintain.

(ruby/irb#824) * Command is not a method * Fix command test * Implement non-method command name completion * Add test for ExtendCommandBundle.def_extend_command * Add helper method install test * Remove spaces in command input parse * Remove command arg unquote in help command * Simplify Statement and handle execution in IRB::Irb * Tweak require, const name * Always install CommandBundle module to main object * Remove considering local variable in command or expression check * Remove unused method, tweak * Remove outdated comment for help command arg Co-authored-by: Stan Lo <[email protected]> --------- ruby/irb@8fb776e379 Co-authored-by: Stan Lo <[email protected]>

tompng force-pushed the command_is_not_a_method branch 2 times, most recently from ab65d7e to 66303ef Compare December 27, 2023 16:50

tompng added the Don't merge label Dec 28, 2023

tompng marked this pull request as ready for review December 28, 2023 20:17

tompng force-pushed the command_is_not_a_method branch from 66303ef to 177e4d4 Compare January 17, 2024 17:19

tompng force-pushed the command_is_not_a_method branch 2 times, most recently from 68002e7 to a6e4401 Compare February 22, 2024 14:16

tompng removed the Don't merge label Feb 22, 2024

tompng force-pushed the command_is_not_a_method branch 2 times, most recently from c6e4fff to b5ca3f2 Compare February 23, 2024 19:20

tompng mentioned this pull request Feb 26, 2024

Support command registration #886

Merged

tompng added 5 commits March 31, 2024 02:41

Command is not a method

a86db06

Fix command test

1069b1c

Implement non-method command name completion

e1595c0

Add test for ExtendCommandBundle.def_extend_command

9c868d2

Add helper method install test

9cfb4fd

tompng force-pushed the command_is_not_a_method branch from b5ca3f2 to 9cfb4fd Compare March 30, 2024 17:51

st0012 reviewed Apr 1, 2024

View reviewed changes

tompng added 2 commits April 1, 2024 22:08

Remove spaces in command input parse

59b52a7

Remove command arg unquote in help command

47c2210

tompng force-pushed the command_is_not_a_method branch from d323b49 to 47c2210 Compare April 1, 2024 13:08

st0012 reviewed Apr 3, 2024

View reviewed changes

tompng added 4 commits April 4, 2024 20:38

Simplify Statement and handle execution in IRB::Irb

eff505e

Tweak require, const name

93b77d2

Always install CommandBundle module to main object

6b6c24b

Remove considering local variable in command or expression check

bc03a96

st0012 reviewed Apr 7, 2024

View reviewed changes

Remove unused method, tweak

1df08b7

st0012 approved these changes Apr 10, 2024

View reviewed changes

lib/irb/command/help.rb Outdated Show resolved Hide resolved

Remove outdated comment for help command arg

128f156

Co-authored-by: Stan Lo <[email protected]>

st0012 merged commit 8fb776e into ruby:master Apr 10, 2024
29 checks passed

This was referenced Apr 10, 2024

[Bug] irb does not allow me to overwrite irb's core methods #839

Closed

Allow alias warnings to be silenced #507

Closed

help(space) errors #905

Closed

kachick mentioned this pull request Apr 11, 2024

2024-04-11 - CI failed in ruby-head(3.4.0dev) kachick/irb-power_assert#183

Closed

st0012 mentioned this pull request Apr 11, 2024

Pass statements to Context#evaluate #920

Merged

tompng deleted the command_is_not_a_method branch April 27, 2024 10:28

st0012 added the enhancement New feature or request label May 1, 2024

st0012 changed the title ~~Command implementation not by method~~ Move away from method-based command implementation May 1, 2024

Earlopain mentioned this pull request May 24, 2024

undefined method `extract_ruby_args=' for module IRB::Command (NoMethodError) #958

Closed

tompng mentioned this pull request Aug 13, 2024

irb warning: "can't alias ls from irb_ls." should perhaps not lead to a warning for vanilla ruby code #341

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move away from method-based command implementation #824

Move away from method-based command implementation #824

tompng commented Dec 24, 2023 •

edited

Loading

st0012 Apr 1, 2024

tompng Apr 1, 2024

st0012 Apr 3, 2024 •

edited

Loading

st0012 Apr 3, 2024

st0012 Apr 3, 2024 •

edited

Loading

st0012 Apr 3, 2024

tompng Apr 4, 2024

st0012 Apr 3, 2024

tompng Apr 4, 2024

st0012 Apr 4, 2024

tompng Apr 4, 2024

st0012 left a comment

	@irb_context.workspace.binding.eval "IRB::ExtendCommand.extract_ruby_args #{arg}"
	@irb_context.workspace.binding.eval "IRB::Command.extract_ruby_args #{arg}"

Move away from method-based command implementation #824

Move away from method-based command implementation #824

Conversation

tompng commented Dec 24, 2023 • edited Loading

No main object method polluting by default

What kind of input is a command?

Command completion

conf and context method

Choose a reason for hiding this comment

Choose a reason for hiding this comment

st0012 Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

st0012 Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

st0012 left a comment

Choose a reason for hiding this comment

tompng commented Dec 24, 2023 •

edited

Loading

`conf` and `context` method

st0012 Apr 3, 2024 •

edited

Loading

st0012 Apr 3, 2024 •

edited

Loading