kotovalexarian-likes-github/pry--pry
1
0
Fork 0
mirror of https://github.com/pry/pry.git synced 2022-11-09 12:35:05 -05:00
Code Releases Activity

Fix Pry.config.hooks # => parent's hooks

Browse source 
PROBLEM DESCRIPTION
===================

The child caches a dup of the parent's hooks,
so it does the correct thing after being accessed the first time.

But the first time it is called, it doesn't return its cached duped value,
it returns the parent's value. This causes 2 bugs:

1. The first hook declared will get "lost", because it is declared
   to the parent hooks, not the child's hooks. And after that first
   accessing, only the child's hooks will be used
2. The parent's hooks (which are probably the ones from
   [the default](0ff6fa61d4/lib/pry/config/default.rb (L35-37)))
   are going to get hooks declared on them that shouldn't be
   (well, presumably shouldn't be, given the explicit check for
   this case and duping to avoid it).
   So if another config object was initialized with the same parent,
   it would wind up with hooks that it didn't want/expect.

I assume all the other keys work fine because they are value objects,
so if anything wanted to change the value, it would reassign the key
to the config object rather than mutating the returned value.

Mutability *sigh* yallknowwhatimsayin

SOLUTION DESCRIPTION
====================

Return the cached duped child value instead of the parent value.

EXAMPLE
=======

This code from
[the docs](0ff6fa61d4/lib/pry/hooks.rb (L8-11))
will not print "hello", because it's being declared to the parent hook.

```ruby
Pry.config.hooks.add_hook(:before_session, :say_hi) { puts "hello" }
```

This code will, b/c it's being declared to the cached, duped child hook.

```ruby
Pry.config.hooks
Pry.config.hooks.add_hook(:before_session, :say_hi) { puts "hello" }
```

ISSUE IMPACT
============

Fixes #1254

Might fix or have something to do with these issues:

* #1244 Totally thought I got it, but then couldn't reproduce again later
  and just don't understand how JRuby builds, so might just be a problem
  with how I was trying to reproduce it.
* #1171 Maybe, depending on if Eclipse integrates with it using the
  hooks, alternatively, could be the same issue as the one above,
  they might both still be broken. Or both fixed and I can't figure out
  how to prove it. Dunno.

OTHER THOUGHTS
==============

I'm writing Github flavoured markdown,
but I'm not actually sure if it will get rendered as such in the PR.
Esp since git considers leading octothorpes as comments and not headers,
and removes them...

Guess we'll see... bottoms up, friends!
This commit is contained in:
Josh Cheek 2014-07-20 16:04:51 -06:00
parent 0ff6fa61d4
commit d8490e8f53
2 changed files with 9 additions and 1 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
lib/pry/config/behavior.rb
View file

@ -54,7 +54,7 @@ module Pry::Config::Behavior
value = @default.public_send(name, *args, &block)
# FIXME: refactor Pry::Hook so that it stores config on the config object,
# so that we can use the normal strategy.
self[key] = value.dup if key == 'hooks'
self[key] = value = value.dup if key == 'hooks'
value
else
nil

8
spec/config_spec.rb
View file

@ -28,6 +28,14 @@ describe Pry::Config do
local3 = Pry::Config.new(local2)
local3.foo.should == 21
end
it "stores a local copy of the parent's hooks upon accessing them" do
parent = Pry::Config.from_hash(hooks: "parent_hooks")
local = Pry::Config.new parent
local.hooks.gsub! 'parent', 'local'
local.hooks.should == 'local_hooks'
parent.hooks.should == 'parent_hooks'
end
end
describe ".from_hash" do