-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Replace static codegen'd factory classes with generic factories #59
Comments
@ReubenBond - Do you mean to add GrainFactory.CreateObjectReference()? |
I was thinking of "ObserverFactory" or "ClientFactory" but yes, pretty much. -----Original Message----- @ReubenBond - Do you mean to add GrainFactory.CreateObjectReference()? |
My personal preference would be to stick to one static factory class, GrainFactory, for better discoverability. |
The proposal is to add public static Task<TGrainObserverInterface> GrainFactory.CreateObjectReference<TGrainObserverInterface>(TGrainObserverInterFace obj); As well as the matching LGTYou? |
What should the behavior be when a user calls Currently it will crash at runtime when it is unable to find the concrete type's factory. I think the best approach is to force the user to specify the interface in the type parameters, which equates to changing the signature to: CreateObjectReference<TGrainObserverInterface>(IGrainObserver obj)
: where TGrainObserverInterface : IGrainObserver Usage then becomes: var obj = await GrainFactory.CreateObjectReference<ISimpleGrainObserver>(observer); I'm performing a runtime check that the argument matches the type parameter: if (!interfaceType.IsInstanceOfType(obj))
{
throw new ArgumentException(
string.Format("The provided object must implement '{0}'.", interfaceType.FullName),
"obj");
} What do you think? |
Do you really need to perform the check at runtime? The compiler won't On Sun, 1 Feb 2015 22:41 Reuben Bond notifications@github.com wrote:
|
I can't have generic parameter restrictions to say To make it less error prone, I changed the type in the parameter list to Maybe I'm missing something, though. Do you have a suggestion for how I can improve this? |
Just an idea, would |
The compile will already enforce At run time, don't we just need to check that it's an interface via |
We do a couple of sanity checks:
The type system cannot infer the interface from the concrete type, so the user must explicitly provide it. We cannot specify a relationship between i.e: // Bad: TInterface is concrete type
Task<TInterface> CreateObjectReference<TInterface>(TInterface obj) where TInterface : IGrainObserver
// Correct, but the user has to specify both types explicitly
Task<TInterface> CreateObjectReference<TInterface,TConcrete>(TConcrete obj) where TInterface : IGrainObserver where TConcrete : TInterface
// Only runtime checks can verify relationship between `obj` and TInterface
Task<TInterface> CreateObjectReference<TInterface>(IGrainObserver obj) where TInterface : IGrainObserver |
Fixes dotnet#59 Add test which includes legacy observer factory. Refactored common delegate generation code into separate MakeFactoryDelegate method.
Currently, we are exposing codegen'd factory classes for IGrainObservers (there may be others, please add them here).
We should create a generic factory which provides access to the codegen'd factories and hide the concrete implementation.
This will help us with Dependency Injection and give us the flexibility to use runtime codegen (eg, via Roslyn)
The text was updated successfully, but these errors were encountered: