adding the -s flag to show_source #770

paulreece · 2023-11-19T14:48:43Z

This enhancement allows a user to add the -s flag if they want to access a methods origin definition. It allows for chaining of multiple esses to further go up the classes as needed.

With the following code:

class Foo
  def foo
  end
end

class Bar < Foo
  def foo
    super
  end
end

If you call show_source Bar#foo -s it will return:

From: test.rb:4

  def foo
  end

Closes #718.

paulreece · 2023-11-19T16:06:47Z

Sorry for all of the pushes! My test file had a namespace clash with the Foo class that was being used by the other tests. I just changed it to Baz instead. :)

st0012

Thank you for the PR! Just few comments about testing.

test/irb/test_cmd.rb

lib/irb/source_finder.rb

tompng · 2023-11-23T17:17:25Z

lib/irb/source_finder.rb

@@ -26,6 +31,13 @@ def find_source(signature)
      when /\A(?<owner>[A-Z]\w*(::[A-Z]\w*)*)#(?<method>[^ :.]+)\z/ # Class#method
        owner = eval(Regexp.last_match[:owner], context_binding)
        method = Regexp.last_match[:method]
+        if esses
+          super_owner = owner
+          s_count.times {|s| super_owner = super_owner.superclass}


If there are too many -ssss, super_owner.superclass raises undefined method `superclass' for nil:NilClass. Adding this to test code would be great

I added this test in there already in the previous commit:

def test_show_source_method_exceeds_super_chain code = <<~RUBY class Baz def foo end end class Bar < Baz def foo super end end RUBY File.write("#{@tmpdir}/bazbar.rb", code) out, err = execute_lines( "irb_load '#{@tmpdir}/bazbar.rb'\n", "show_source Bar#foo -ss", ) assert_match(%r[Error: Couldn't locate a definition for Bar#foo -ss\n=> nil\n], out) end

Does that cover the use case you're referring to? If not I'm happy to add another.

lib/irb/source_finder.rb

lib/irb/cmd/show_source.rb

paulreece · 2023-11-25T15:56:36Z

FYI: the truffleruby tests seem to be erroring out with some gem dependency errors

lib/irb/cmd/show_source.rb

st0012

I have a few ideas for small improvements and adopting the same flag to edit. But I think it's good to merge now. I'll let @tompng do another pass before pushing the button though.

@paulreece Thank you so much for the work on this PR, as well as all the feedback you provided during RubyConf ❤️

paulreece · 2023-11-25T19:35:34Z

@paulreece Thank you so much for the work on this PR, as well as all the feedback you provided during RubyConf ❤️

@st0012 you're welcome! It was a pleasure working with you and @tompng on this PR. I'm happy to try to address other items from the RubyConf issues or other open bug issues if you'd like. I really enjoy how constructive working in this repo is. :)

st0012 · 2023-11-25T20:39:25Z

@paulreece It's not listed in the issues yet, but I'd really like to make show_source command provide more metadata about the method, like the ones from Pry:

Visibility: public
Signature: baz()
Number of lines: 3

You are definitely welcome to work on any other issues :-) But this one just came up to me today.

paulreece · 2023-11-25T22:16:26Z

It's not listed in the issues yet, but I'd really like to make show_source command provide more metadata about the method, like the ones from Pry:
Visibility: public
Signature: baz()
Number of lines: 3

Awesome! I'll get started on this one!

lib/irb/source_finder.rb

st0012 · 2023-11-26T19:17:47Z

lib/irb/source_finder.rb

        end
+        methods = owner.instance_methods + owner.private_instance_methods
+        file, line = owner.instance_method(method).source_location if methods.include?(method.to_sym)


Sorry that I only found this now: I think it's also possible to get the method through super_method? Something like:

target_method = owner.instance_method(method) s_count.times do |s| target_method = target_method.super_method if target_method end file, line = target_method.source_location if target_method

st0012 · 2023-11-26T20:26:53Z

lib/irb/source_finder.rb

        end
+        methods = owner.instance_methods + owner.private_instance_methods
+        file, line = owner.instance_method(method).source_location if methods.include?(method.to_sym)
      when /\A((?<receiver>.+)(\.|::))?(?<method>[^ :.]+)\z/ # method, receiver.method, receiver::method
        receiver = eval(Regexp.last_match[:receiver] || 'self', context_binding)


If we can use the above approach, we can support these signatures too.

Just so I'm understanding this correctly. We would be combining the two different case statements here when I implement the above approach? Or would I be adding this target_method logic into each one?

when /\A(?<owner>[A-Z]\w*(::[A-Z]\w*)*)#(?<method>[^ :.]+)\z/ # Class#method when /\A((?<receiver>.+)(\.|::))?(?<method>[^ :.]+)\z/

Not combining the case statements. I imagine you can extract the super lookup logic into a private method and use it in both cases instead.

Implemented this and refactored find_source a bit as well. Lmk what you think!

tompng · 2023-11-27T11:29:03Z

test/irb/test_cmd.rb

+        "irb_load '#{@tmpdir}/bazbar.rb'\n",
+        "show_source Bar#foo -s",
+      )
+      assert_match(%r[Error: Couldn't locate a super definition for Bar#foo\n], out)


CI (ruby-core setup) is failing in this line.
You can reproduce it by removing def test_show_source_method_multiple_s and run rake test TEST=test/irb/test_cmd.rb in local.

The problem is that after other test, Object::Baz still remains with method foo defined.
In this case, class Baz; end is not a class definition but an open-class.
The test accidentally passed in other CI because source_location file bazbarbob.rb was already deleted.

I think the easiest way to fix this is to add these in all added tests

ensure Object.remove_const :Bar Object.remove_const :Baz Object.remove_const :Bob end

I ended up just changing the method names in each test since it wouldn't let me access the remove_const method now these pass even when commenting out def test_show_source_method_multiple_s

TruffleRuby CI is failing because of a gem I believe:

In Gemfile: debug was resolved to 1.8.0, which depends on irb was resolved to 1.9.1, which depends on rdoc was resolved to 6.6.0, which depends on psych was resolved to 5.1.1.1, which depends on stringio Error: The process '/home/runner/.rubies/truffleruby-head/bin/bundle' failed with exit code 5

(Don't worry about truffleruby for now, we're aware of the problem and have contacted related teams)

…ess a methods origin definition. It allows for chaining of multiple esses to further go up the classes as needed.

st0012

Thank you so much!

want to access a methods origin definition. It allows for chaining of multiple esses to further go up the classes as needed. (ruby/irb#770) ruby/irb@eec1329d5a

jemmaissroff · 2023-11-28T17:08:18Z

👋 Looks like this PR has introduced test failures in the ruby/ruby master build (https://github.com/ruby/ruby/commits/master). Flagging in case you weren't aware

st0012 · 2023-11-28T17:10:25Z

Thanks for flagging it. I've just addressed the issues (hopefully) in #793. But I think the commit was not synced to ruby/ruby?

jemmaissroff · 2023-11-28T17:14:41Z

Hmm interesting, it does seem it wasn't synced. Oddly too, it doesn't appear in the #alerts-sync channel in RubyLang slack which usually indicates sync failures. Perhaps we can try syncing it manually?

paulreece · 2023-11-28T17:59:46Z

Sorry about that y'all. @st0012 Do you have any suggestions for the next PR I open in order to avoid the bug issues that the tests caused on this one? Maybe I setup the tests a bit differently?

st0012 · 2023-11-28T18:11:59Z

Just add new tests to the new file added in #793 should be enough 😄

paulreece changed the title … adding the s flag to show_source Nov 19, 2023

paulreece force-pushed the add_s_to_show_source branch 6 times, most recently from 5f8bc7a to e42514d Compare November 19, 2023 16:05

paulreece force-pushed the add_s_to_show_source branch from e42514d to 6e361f1 Compare November 19, 2023 16:08

paulreece changed the title ~~adding the s flag to show_source~~ adding the -s flag to show_source Nov 19, 2023

st0012 reviewed Nov 19, 2023

View reviewed changes

test/irb/test_cmd.rb Outdated Show resolved Hide resolved

test/irb/test_cmd.rb Outdated Show resolved Hide resolved

test/irb/test_cmd.rb Show resolved Hide resolved

st0012 added the enhancement New feature or request label Nov 19, 2023

paulreece force-pushed the add_s_to_show_source branch 3 times, most recently from 117aa20 to 2839617 Compare November 20, 2023 22:30

st0012 reviewed Nov 22, 2023

View reviewed changes

test/irb/test_cmd.rb Outdated Show resolved Hide resolved

lib/irb/source_finder.rb Outdated Show resolved Hide resolved

lib/irb/source_finder.rb Outdated Show resolved Hide resolved

tompng reviewed Nov 23, 2023

View reviewed changes

lib/irb/source_finder.rb Outdated Show resolved Hide resolved

paulreece force-pushed the add_s_to_show_source branch 2 times, most recently from 7476427 to ee22f4a Compare November 24, 2023 17:53

ima1zumi reviewed Nov 25, 2023

View reviewed changes

lib/irb/cmd/show_source.rb Outdated Show resolved Hide resolved

paulreece force-pushed the add_s_to_show_source branch from ee22f4a to f27f369 Compare November 25, 2023 15:54

st0012 reviewed Nov 25, 2023

View reviewed changes

lib/irb/cmd/show_source.rb Outdated Show resolved Hide resolved

paulreece force-pushed the add_s_to_show_source branch 2 times, most recently from 1918cfa to 631de04 Compare November 25, 2023 18:38

st0012 approved these changes Nov 25, 2023

View reviewed changes

tompng reviewed Nov 26, 2023

View reviewed changes

lib/irb/source_finder.rb Outdated Show resolved Hide resolved

paulreece force-pushed the add_s_to_show_source branch from 631de04 to ded61d1 Compare November 26, 2023 17:58

st0012 reviewed Nov 26, 2023

View reviewed changes

tompng reviewed Nov 27, 2023

View reviewed changes

This enhancement allows a user to add the -s flag if they want to acc…

1576254

…ess a methods origin definition. It allows for chaining of multiple esses to further go up the classes as needed.

paulreece force-pushed the add_s_to_show_source branch from ded61d1 to 1576254 Compare November 27, 2023 22:54

tompng approved these changes Nov 28, 2023

View reviewed changes

st0012 approved these changes Nov 28, 2023

View reviewed changes

st0012 merged commit eec1329 into ruby:master Nov 28, 2023
22 of 24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding the -s flag to show_source #770

adding the -s flag to show_source #770

paulreece commented Nov 19, 2023 •

edited

Loading

paulreece commented Nov 19, 2023

st0012 left a comment

tompng Nov 23, 2023 •

edited

Loading

paulreece Nov 24, 2023

paulreece commented Nov 25, 2023

st0012 left a comment

paulreece commented Nov 25, 2023

st0012 commented Nov 25, 2023

paulreece commented Nov 25, 2023

st0012 Nov 26, 2023

st0012 Nov 26, 2023

paulreece Nov 26, 2023

st0012 Nov 26, 2023

paulreece Nov 26, 2023

paulreece Nov 27, 2023

tompng Nov 27, 2023

paulreece Nov 27, 2023

st0012 Nov 27, 2023

st0012 left a comment

jemmaissroff commented Nov 28, 2023

st0012 commented Nov 28, 2023 •

edited

Loading

jemmaissroff commented Nov 28, 2023

paulreece commented Nov 28, 2023

st0012 commented Nov 28, 2023

adding the -s flag to show_source #770

adding the -s flag to show_source #770

Conversation

paulreece commented Nov 19, 2023 • edited Loading

paulreece commented Nov 19, 2023

st0012 left a comment

Choose a reason for hiding this comment

tompng Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulreece commented Nov 25, 2023

st0012 left a comment

Choose a reason for hiding this comment

paulreece commented Nov 25, 2023

st0012 commented Nov 25, 2023

paulreece commented Nov 25, 2023

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

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

jemmaissroff commented Nov 28, 2023

st0012 commented Nov 28, 2023 • edited Loading

jemmaissroff commented Nov 28, 2023

paulreece commented Nov 28, 2023

st0012 commented Nov 28, 2023

paulreece commented Nov 19, 2023 •

edited

Loading

tompng Nov 23, 2023 •

edited

Loading

st0012 commented Nov 28, 2023 •

edited

Loading