fix #1471, fix #1621 by 0x1eef · Pull Request #1689 · pry/pry

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

        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

        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 ConradIrwin, banister 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

Conversation

0x1eef commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0x1eef commented Nov 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

banister commented Nov 8, 2017

Uh oh!

0x1eef commented Nov 8, 2017

Uh oh!

0x1eef commented Nov 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0x1eef commented Nov 8, 2017

Uh oh!

0x1eef commented Nov 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0x1eef Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0x1eef commented Nov 8, 2017

Uh oh!

0x1eef commented Nov 8, 2017

Uh oh!

0x1eef commented Nov 8, 2017

Uh oh!

banister left a comment

Choose a reason for hiding this comment

Uh oh!

0x1eef commented Nov 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

0x1eef commented Nov 7, 2017 •

edited

Loading

0x1eef Nov 8, 2017 •

edited

Loading