fix #1471, fix #1621 #1689

0x1eef · 2017-11-07T23:55:49Z

Abort early when searching for a superclass if the target method
has been called through 'super' keyword from a prepended module.

Requires review, and tests.

automated test case
verify fixes the test case in issue in Pry doesn't show code when prepended modules are used #1471
verify fixes the test hanami application in Pry does not show code in Hanami's #call #1621

0x1eef · 2017-11-08T00:06:32Z

ref #1621

banister · 2017-11-08T00:13:48Z

lib/pry/method/weird_method_locator.rb

+        target_mod = @target.eval('self').class
+        target_mod.ancestors.take_while do |mod|
+          mod != target_mod
+        end.size > 0


Nice code (re take_while)! but can replace this with .any? I think..

It's not equivalent. Introduces two failures:

1) Pry::Method.from_binding should find the super method correctly Failure/Error: expect(m.owner).to eq a expected: #<Class:0x007ff166802c30> got: #<Class:0x007ff166802b40> (compared using ==) Diff: @@ -1,2 +1,2 @@ -#<Class:0x007ff166802c30> +#<Class:0x007ff166802b40> # ./spec/method_spec.rb:130:in `block (3 levels) in <top (required)>' 2) Pry::Method.from_binding should find the right method even if it was renamed and replaced Failure/Error: expect(m.source).to eq Pry::Method(o.method(:paella)).source expected: "def borscht\n @nips = \"nips\"\n binding\nend\n" got: "def borscht() paella end\n"

@R-obert sorry i wasn't clear. I didn't mean replace the take_while i meant to replace the size > 0 with any?

oh. sure. i'll do that.

banister · 2017-11-08T00:18:02Z

lib/pry/method/weird_method_locator.rb

-          File.expand_path(method.source_file) == File.expand_path(b.eval('__FILE__')) &&
-          method.source_range.include?(b.eval('__LINE__'))
+          if method and method.source_file and method.source_range
+            binding_FILE, binding_LINE = b.eval('__FILE__'), b.eval('__LINE__')


I think binding_file is nicer than binding_FILE though i can see what you were going for here, i still personally prefer binding_file

banister · 2017-11-08T00:25:12Z

How does it relate to this PR is it possible to fix this one too? #1444

0x1eef · 2017-11-08T00:35:30Z

updated. not sure about the other issue.

0x1eef · 2017-11-08T00:36:42Z

i think treat #1444 separately.

banister · 2017-11-08T01:10:39Z

lib/pry/method/weird_method_locator.rb

@@ -117,6 +123,11 @@ def find_method_in_superclass
        nil
      end

+      def prepended_method?(guess)


Don't we also need to check that the method is defined on the prepended module?

sorry dude, very sleepy. i think so. i'll return to this soon. busy day tomorrow 😉

0x1eef · 2017-11-08T10:56:23Z

@banister woke up with some coffee. d6a6d0d

Abort early when searching for a superclass if the target method has been called through 'super' keyword from a prepended module.

0x1eef · 2017-11-08T11:21:45Z

8ee58a0 is the (hopefully) final fix in this PR.
i will add an automated test-case. all tests passing locally.

banister · 2017-11-08T11:29:07Z

lib/pry/method/weird_method_locator.rb

@@ -98,7 +107,9 @@ def pry_file?
      #   superclass method.
      def find_method_in_superclass
        guess = method
-
+        if skip_superclass_search?
+          return guess


Nice, but the purpose of this class is to return the correct method for the binding, does this achieve this?

It returns a Pry::Method for the prepended module:

[1] pry(#<Pry::Method::WeirdMethodLocator>)> guess => #<Pry::Method:0x007ff2090a99e0 @method=#<Method: Foo(AlsoPrependThis)#foo>, @source="def foo\n super + 2\nend\n", @visibility=nil>

For the Hanami case this does not seem to matter.

0x1eef · 2017-11-08T14:54:36Z

test case added, let's see if it passes:
c1a717c

0x1eef · 2017-11-08T14:57:01Z

force pushed to:
b0bc7ed

0x1eef · 2017-11-08T15:11:05Z

@banister
please review, and consider a merge/point release.
i think your comment #1689 (comment) is related to #1444 and should be treated separately.

The point mentioned relates to #1444 and I believe it should be tackled separately to give Hanami users a pleasant Pry experience in the shorter term. It is true that whereami should report a different owner, but it does not effect functionality of the binding used. In addition the method altered will most likely always need a shortcut method similar to skip_superclass_search?.

banister

Nice work! Let's get this shipped asap and see what hanami people report back to us then

0x1eef · 2017-11-11T00:14:14Z

@banister all yours! ⭐ There's a number of bug fixes that could go into the next point release of 0.11. See CHANGELOG.md for details.

pkgsrc change: add support for pkg_alternatives ### HEAD #### Features * Add Pry::Testable, an improved modular replacement for PryTestHelpers. **breaking change**. See pull request [#1679](pry/pry#1679). * Add a new category module: "Pry::Platform". Loosely related to #1668 below. See pull request [#1670](pry/pry#1670) * Add `mac_osx?` and `linux?` utility functions to Pry::Helpers::BaseHelpers. See pull request [#1668](pry/pry#1668). * Add utility functions for drawing colorised text on a colorised background. See pull request [#1673](pry/pry#1673). #### Bug fixes * Fix a case of infinite recursion in `Pry::Method::WeirdMethodLocator#find_method_in_superclass` that users of the [Hanami](http://hanamirb.org/) web framework experienced and reported since 2015. See pull request [#1639](pry/pry#1689). * Fix a bug where Method objects were not returned for setters inherited from a default (Pry::Config::Default). Eg, this is no longer an error: pry(main)> d = Pry::Config.from_hash({}, Pry::Config::Default.new) pry(main)> d.method(:exception_whitelist=) # Error See pull request [#1688](pry/pry#1688). * Do not capture unused Proc objects in Text helper methods `no_color` and `no_paging`, for performance reasons. Improve the documentation of both methods. See pull request [#1691](pry/pry#1691). * Fix `String#pp` output color. See pull request [#1674](pry/pry#1674). ### 0.11.0 * Add alias 'whereami[?!]+' for 'whereami' command. ([#1597](pry/pry#1597)) * Improve Ruby 2.4 support ([#1611](pry/pry#1611)): * Deprecated constants are hidden from `ls` output by default, use the `-d` switch to see them. * Fix warnings that originate in Pry while using the repl. * Improve completion speed in large applications. ([#1588](pry/pry#1588)) * Pry::ColorPrinter.pp: add `newline` argument and pass it on to PP. ([#1603](pry/pry#1603)) * Use `less` or system pager pager on MS Windows if it is available. ([#1512](pry/pry#1512)) * Add `Pry.configure` as an alternative to the current way of changing configuration options in `.pryrc` files. ([#1502](pry/pry#1502)) * Add `Pry::Config::Behavior#eager_load!` to add a possible workaround for issues like ([#1501](pry/pry#1501)) * Remove Slop as a runtime dependency by vendoring v3.4 as Pry::Slop. People can depend on Slop v4 and Pry at the same time without running into version conflicts. ([#1497](pry/pry#1497)) * Fix auto-indentation of code that uses a single-line rescue ([#1450](pry/pry#1450)) * Remove "Pry::Config#refresh", please use "Pry::Config#clear" instead. * Defining a method called "ls" no longer breaks the "ls" command ([#1407](pry/pry#1407)) * Don't raise when directory permissions don't allow file expansion ([#1432](pry/pry#1432)) * Syntax highlight <tt> tags in documentation output. * Add support for BasicObject subclasses who implement their own #inspect (#1341) * Fix 'include RSpec::Matchers' at the top-level (#1277) * Add 'gem-readme' command, prints the README file bundled with a rubygem * Add 'gem-search' command, searches for a gem with the rubygems.org HTTP API * Fixed bug in the `cat` command where it was impossible to use line numbers with files ([#1349](pry/pry#1349)) * Fixed uncaught Errno::EOPNOTSUPP exception when $stdout is a socket ([#1352](pry/pry#1352)) * Display a warning when you cd'ed inside a C object and executed 'show-source' without arguments ([#691](pry/pry#691)) * Make the stagger_output method more reliable by reusing possibly available Pry instance ([#1364](pry/pry#1364)) * Make the 'gem-install' message less confusing by removing backticks ([#1350](pry/pry#1350)) * Fixed error when Pry was trying to load incompatible versions of plugins ([#1312](pry/pry#1312)) * Fixed bug when `hist --clear` led to ArgumentError ([#1340](pry/pry#1340)) * Fixed the "uninitialized constant Pry::ObjectPath::StringScanner" exception during autocomplete ([#1330](pry/pry#1330)) * Secured usage of colours with special characters (RL_PROMPT_START_IGNORE and RL_PROMPT_END_IGNORE) in Pry::Helpers::Text ([#493](pry/pry#493 (comment))) * Fixed regression with `pry -e` when it messes the terminal ([#1387](pry/pry#1387)) * Fixed regression with space prefixes of expressions ([#1369](pry/pry#1369)) * Introduced the new way to define hooks for commands (with `Pry.hooks.add_hook("{before,after}_commandName")`). The old way is deprecated, but still supported (with `Pry.commands.{before,after}_command`) ([#651](pry/pry#651)) * Removed old API's using `Pry::Hooks.from_hash` altogether * Removed hints on Foreman support (see [this](ddollar/foreman#536)) * Fixed support for the tee command ([#1334](pry/pry#1334)) * Implemented support for CDPATH for ShellCommand ([#1433](pry/pry#1433), [#1434](pry/pry#1434)) * `Pry::CLI.parse_options` does not start Pry anymore ([#1393](pry/pry#1393)) * The gem uses CPU-less platforms for Windows now ([#1410](pry/pry#1410)) * Add `Pry::Config::Memoization` to make it easier to implement your own `Pry::Config::Default` class.([#1503](pry/pry#1503)) * Lazy load the config defaults for `Pry.config.history` and `Pry.config.gist`.

0x1eef mentioned this pull request Nov 7, 2017

Pry doesn't show code when prepended modules are used #1471

Closed

0x1eef requested review from banister, ConradIrwin and rf- November 7, 2017 23:58

0x1eef mentioned this pull request Nov 8, 2017

Pry does not show code in Hanami's #call #1621

Closed

banister previously requested changes Nov 8, 2017

View reviewed changes

0x1eef changed the title ~~Fix #1471~~ fix #1471, fix #1621 Nov 8, 2017

banister reviewed Nov 8, 2017

View reviewed changes

fix #1471, fix #1621

8ee58a0

Abort early when searching for a superclass if the target method has been called through 'super' keyword from a prepended module.

banister reviewed Nov 8, 2017

View reviewed changes

r-obert added 2 commits November 8, 2017 15:55

mv spec/regression spec/integration

1f318f4

add hanami integration spec

b0bc7ed

R-obert and others added 3 commits November 8, 2017 19:34

Merge branch 'master' into bugfix/prepend_infinite_loop

4e6d767

update CHANGELOG.md

a8d87af

update CHANGELOG.md

c4e6be6

0x1eef self-assigned this Nov 10, 2017

banister approved these changes Nov 10, 2017

View reviewed changes

R-obert added 2 commits November 11, 2017 00:53

Merge branch 'master' into bugfix/prepend_infinite_loop

7957643

Merge branch 'master' into bugfix/prepend_infinite_loop

a0484dc

0x1eef merged commit 6a89f57 into pry:master Nov 11, 2017

0x1eef deleted the bugfix/prepend_infinite_loop branch November 11, 2017 00:12

dependabot-preview bot mentioned this pull request Mar 16, 2018

Bump pry from 0.10.4 to 0.11.3 bustikiller/raffle-bee#22

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #1471, fix #1621 #1689

fix #1471, fix #1621 #1689

0x1eef commented Nov 7, 2017 •

edited

Loading

0x1eef commented Nov 8, 2017

banister Nov 8, 2017

0x1eef Nov 8, 2017

banister Nov 8, 2017

0x1eef Nov 8, 2017

0x1eef Nov 8, 2017

banister Nov 8, 2017

banister commented Nov 8, 2017

0x1eef commented Nov 8, 2017

0x1eef commented Nov 8, 2017

banister Nov 8, 2017

0x1eef Nov 8, 2017

0x1eef commented Nov 8, 2017

0x1eef commented Nov 8, 2017

banister Nov 8, 2017

0x1eef Nov 8, 2017 •

edited

Loading

0x1eef commented Nov 8, 2017

0x1eef commented Nov 8, 2017

0x1eef commented Nov 8, 2017

banister left a comment

0x1eef commented Nov 11, 2017

fix #1471, fix #1621 #1689

fix #1471, fix #1621 #1689

Conversation

0x1eef commented Nov 7, 2017 • edited Loading

0x1eef commented Nov 8, 2017

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

banister commented Nov 8, 2017

0x1eef commented Nov 8, 2017

0x1eef commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x1eef commented Nov 8, 2017

0x1eef commented Nov 8, 2017

Choose a reason for hiding this comment

0x1eef Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

0x1eef commented Nov 8, 2017

0x1eef commented Nov 8, 2017

0x1eef commented Nov 8, 2017

banister left a comment

Choose a reason for hiding this comment

0x1eef commented Nov 11, 2017

0x1eef commented Nov 7, 2017 •

edited

Loading

0x1eef Nov 8, 2017 •

edited

Loading