Evaluate default procs with the interaction's binding #332
Conversation
Both the tests and the documentation aren't happy about this.
|
@AaronLasseigne can you review this? It's not ready to go yet. I still need to do documentation. But this is working and it was less work than I expected. The only commit worth looking at is 87a43e7. |
|
The general approach is good. I played with it a bit and it seems to work well. I know you said it's not entirely done but I have a few nitpicks that I'll add. |
| # | ||
| # @return [Object] | ||
| # | ||
| # @raise (see #cast) | ||
| # @raise (see #default) | ||
| def clean(value) | ||
| value = cast(value) | ||
| def clean(value, interaction) |
There was a problem hiding this comment.
Rather than refer to this as interaction how about context. That's what it really is, right?
There was a problem hiding this comment.
Or binding might be even better.
There was a problem hiding this comment.
I thought about binding, but it's not actually a Binding object. I prefer interaction over context because it's always an interaction. "Context" may kind of explain what it's doing, but "interaction" explains what it is.
That being said, it might be better to pass a duplicate of the interaction's binding instead of the interaction itself. That would prevent you from modifying the interaction inside the default proc.
There was a problem hiding this comment.
Never mind. Using a binding won't work because then self.foo would mean interaction.binding.foo instead of interaction.foo.
There was a problem hiding this comment.
I still think that we're running the code in a binding or context regardless of what that context is. The fact that it's an interaction is secondary.
|
BTW, I think this is a feature enhancement so a minor release seems right to me. |
|
I renamed |
| @@ -205,12 +209,14 @@ def describe(value) | |||
| "(Object doesn't support #inspect)" | |||
| end | |||
|
|
|||
| # @param context [Base, nil] | |||
There was a problem hiding this comment.
This can't be nil anymore, right?
There was a problem hiding this comment.
It never is, but it can be. When we call default in eager_evaluate_default, we don't have an interaction to pass in.
If the default is a Proc, then the context is never nil. That's hard to express, though.
|
|
(once we have docs)
|
I added some documentation. |
|
Looks good. |
Evaluate default procs with the interaction's binding
Fixes #217.
This is the latest in a long line of attempts.