Pry.config.* should not use method missing. #1277
Comments
|
TL; DR Getting to the problem point in PryThis isn't necessary for understanding the problem, but given that I did the work to figure out how to get to where the problem is, I figured I'd go ahead and post it anyway. Maybe it will be useful context, or maybe just a thing I can refer back to next time I go traipsing around this codebase, and it's easy enough to skip if you're just interested in the problem.
Understanding the problem point in PrySo, based on the above, we ultimately boil down to
Understanding the problem point in RSpec
What Pry should do
What @ogrishman should do
|
|
Great explanation! Thank you so much for the code analysis. Learned a lot from it. |
|
Can we close this? If we want to follow my preference of removing method_missing, we can open another issue about it and reference this. |
ConradIrwin
changed the title
|
Updated the title to reflect your intention. I'd love to see method_missing removed. |
|
Cool, I'm happy to look at it over the next few days/week, unless anyone else is really excited about it (happy to collaborate, too!) I don't have a whole lot of context regarding use-cases, is there anything I should know? |
I probably won't complete thisSo, it's been a week, I started on this, but haven't looked at it since last weekend. It's rough because I'm trying to balance either (1) don't break existing stuff, (2) minimal breaks to existing stuff, and mitigate by finding what would break and just going and fixing them (ie download all gems, filter to candidates that are likely hit, send them PRs), (3) the concern that configuration can simply never be correct without breaking existing stuff. Anyway, based on my excitement levels when considering various topics, it's very improbable that I'll complete this. Figured state that since it's shitty to say you'll do something and then just let it drag out forever with people waiting. Some notes on the issueIn order to not have wasted a lot of effort, here's a handful of notes, I have more, but they're all over the place and likely useless. A good Idea that I can't quite rememberOh, and I think @rf- made a really suggestion on IRC about... uhm.. maybe defining methods on singleton class? Don't remember exactly, just remember being a little inspired, thinking it could work and dramatically minimize breaking the public api. Mutability + nesting = situations with no correct answerAnyway, here's some examples of the issues. I'm not sure if all the issues boil down to this, or if there's several around this theme, since I didn't finish consolidating my ideas. require 'pry' # => true
config = Pry::Config.new # => #<Pry::Config:0x3fd53cd93db0� local_keys=[] default=#<Pry::Config:0x3fd53d92cf00� local_keys=['hooks'] default=#<Pry::Config::Default:0x3fd53d92d1bc� local_keys=['gist','history'] default=nil>>>
# mutating the child is seen by the parent
config.extra_sticky_locals[:a] = 1 # => 1
Pry.extra_sticky_locals # => {:a=>1}
# assignments on the child are *NOT* seen by the parent
config.extra_sticky_locals = {a: 2} # => {:a=>2}
Pry.extra_sticky_locals # => {:a=>1}It's common (for me, at least) to edit Incorrect test?Also, this test Line 50 in 7e73e72 default, not itself (I assume it passes because they are just incidentally ==)
Direction I think it should goI generally think that when we're ready to break the public api (ie next major release), we need to require that all access to config goes through the instance. And we need to lose the nestedness of the configurations because I think there are just some situations where there is simply no right answer (ie it depends on what the user intended in their brain). In my brain it looks something like "we'll pass the pry instance to all plugins at the appropriate time so they can initialize themselves and any config (all other hooks would belong on the individual config)" and then, if this is inconvenient to access for users, then we can provide a command to make it more convenient A musingLast thought, I expect everyone to hate this, but want to state it: I strongly prefer explicitness. With our current setup, assuming we remove the nesting and require all changes to happen to the pry instance's config (to mitigate the mutability issues), there's no reason to not just use a hash. But if we wanted to be more explicit, an object would make sense in that you could require config options to be declared. It then runs some validations like "this doesn't conflict with other config keys", and maybe allows for namespacing, IDK, and then you can use it, and it will blow up if you try to set an undeclared config key (under the assumption that it's a spelling error, probably telling you what you could have set in the error message). Keep in mind, though, that I've never seen this actually done, so I don't know that it would work out well, it just appeals to my design sense. |
|
hey guys, I fixed this issue in 27e9199 by using a similar strategy to OpenStruct and as mentioned by @rf-. @JoshCheek thanks for thorough investigation, it was helpful. |
In an IRB session I can do:
When I try to do the same thing in pry, I got errors.
Below are some of my versions.
Thanks.
The text was updated successfully, but these errors were encountered: