-
Notifications
You must be signed in to change notification settings - Fork 713
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BindingToSyntax refactor discussion #1327
Comments
@notaphplover What do you think ? |
Hi @tonyhallett, I think this is a good question and it's not easy to find a good answer. I'll do my best to give you my opinion. I think this request is a proposal to make an abstraction to avoid duplicated code. I think it's good to remove duplicated code if we can make an abstracion: "those X pieces of code represents exactly the same, so we can write a common one and call it instead of having those X pieces". But I wonder if this case is the same, it depends on multiple factors:
Sometimes we have duplicated code to represent different concepts. This image allways comes to my mind in these cases even if it's used to explain the Liskov substitution principle: So I think answering your question is equivalent to answering the next question: Are the subtasks of building a I think the answer is "Yes, but we got the wrong abstracion". I realize binding is a wrapper to be used for anything. Resolution flows uses this class but the logic is too complex. Resolver has to know which is the right property to use to get a result. Why? The binding itself could take that responsability. I think the duplicated code we are facing is a smell of a wrong abstraction. Your proposal solves the fact of having duplicated code, but we shouldn't give a method a hint of which bindingStoreProperty is the right one to be used, we should try to give Binding a method to provide the solution we need. As an example, if we have a |
I will respond in detail later but yes I agree that there should be a base binding as I mentioned here I have coded a solution that appears to work, probably not the best as it was a proof of concept that is backwards compatible. There is now a BindingBase abstract class. Note that I was rapidly coding on top of my new scope code so it will look ever so slightly different. I have marked with asterisks the new scope code.
With my code, binding has a single responsibility of providing the value when it is not coming from a cache. The "cache" is available on the binding as
Back to binding derivations !
BindingToSyntax
container changes.
resolver - this has some new scope code
Much nicer |
I will create a pull request for the Scope abstraction tomorrow. |
hi @tonyhallett sorry for the late reply. I've had a look, the only problem I see is the class TransientScope<T> implements interfaces.Scope<T>{
get(): T|null {
return null
}
set(_:interfaces.Binding<T>,__:interfaces.Request,resolved:T|Promise<T>):T | Promise<T> {
return resolved;
}
} get() returns null and set does not store anything. I think there's no need to add a TransientScope class. I would add a But sure, go ahead, I think your approach is really good and we can allways iterate |
Not at all !
We could have no TransientScope and have resolveScope null and have the resolver check for null but I think the resolver should be ignorant. I also like it because it is the definition of a transient scope and you can compare scopes side by side and see exactly what they mean.. It also keeps caching out of the Binding inheritance.
Cool, I will make the pull request/s tomorrow and if for any reason it comes up short we can try other avenues. Two pull requests ? Scope first then binding inheritance ? |
All right :)
I think they those Scopes are related to bindings and iterations could be easier if we submit only one pull request, so I would suggest a single pull request |
@notaphplover Will have something to show tomorrow. |
There is common code in each of the "to" methods. For instance
Common
We set the type.
We reset the properties that do not apply.
We set the scope to singleton ( this is common for some of these "to" methods )
Non custom
We set the applicable prroperty - for toConstantValue it is the cache
Common
Return new BindingWhenOnSyntax with the binding
Solution
`
public toConstantValue(value: T): interfaces.BindingWhenOnSyntax {
return this._setBinding(BindingTypeEnum.ConstantValue,true,"cache",value);
}
`
The text was updated successfully, but these errors were encountered: