-
Notifications
You must be signed in to change notification settings - Fork 33
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Just need to ensure we can add the attribute to everything and be able to register globally for classes we have no control over. string etc...
/// The Ditto lazy property attribute. | ||
/// Used for specifying that Ditto should lazy load this property. | ||
/// </summary> | ||
[AttributeUsage(AttributeTargets.Property | AttributeTargets.Class, AllowMultiple = false)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allow on enum and interface too. We have a constant now don't we for this?
There's also a way we can globally register isn't there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a confusion I need to check here. Having the attribute declared on a type currently makes all of it's contained virtual properties lazy, so it could make sense to use on an Interface (assuming you can declare properties as virtual on an interface) but wouldn't so much on an Enum.
I did have this discussion with Lee, but would be good to get your view point. What are you expecting the class level Lazy attribute to do? Make all it's inner properties lazy? or make the parent declaring property lazy? (ie, the parent property that defines it's type as your type with the lazy attribute applied). If it's the later, how would this affect enumerable props?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd expect it to work in the same way as other attributes. I.E if I mark an enum with the EnumAttribute
then every time I declare that enum type as a property I know the processor will kick in without me having to declare the attribute again. Same as with the UmbracoPickerAttribute
. I declare any base classes with that attribute so I can use them as properties at any time and always have a working picker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn your brain! Now I think about it really like that you can declare all properties within a class as lazy. That's a really neat way to do it and way better than what I was thinking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, cool, well worth f you are happy with way too, let's go with it. I think it's the easiest to comprehend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, that should have been "well if you are happy"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this approach, would we still want a Ditto.RegisterLazyAttribute function to register the class level ditto lazy on types you may not have control over? (again, this would just make that types inner virtual properties lazy, not the type itself)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we'll need to. There's plenty of fine grained control already now.
{ | ||
continue; | ||
// Ensure it's a virtual property (Only relevant to property level lazy loads) | ||
if (!propertyInfo.IsVirtualAndOverridable()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good check.
Thanks @JimBobSquarePants. We don't have the global register / global flag yet, but should be easy enough to add. |
…properties to be lazy, or attributed properties only to be lazy
Added some starter unit tests but I could do with some advice from @JimBobSquarePants as to how we can verify in code that a given property has an interceptor registered? |
…hin the ShouldAttemptLazyLoad extension method
…zy attribute was added as it needs to be called after the lazy proxy is created
@mattbrailsford You should be able to check whether the result implements |
Yea, currently doing that, but no easy way to "check" which properties are
setup to be proxied.
|
As discussed in #190, adding a DittoLazy attribute to be explicit about which properties are Lazy loaded.
@JimBobSquarePants can you check at line 385 that this is ok? In the original code, virtual props were made first, and this code ran, THEN non virtual property values were set. Should this be ok that all properties are set first lazy and non lazy, and THEN the proxy object is created?
Still need to do some unit tests