Skip to content
This repository has been archived by the owner on Dec 13, 2021. It is now read-only.

DittoLazy attribute #191

Merged
merged 11 commits into from Sep 21, 2016
Merged

DittoLazy attribute #191

merged 11 commits into from Sep 21, 2016

Conversation

mattbrailsford
Copy link
Collaborator

@mattbrailsford mattbrailsford commented Sep 16, 2016

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

Copy link

@JimBobSquarePants JimBobSquarePants left a 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)]

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?

Copy link
Collaborator Author

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?

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.

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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"

Copy link
Collaborator Author

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)

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())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good check.

@mattbrailsford
Copy link
Collaborator Author

Thanks @JimBobSquarePants. We don't have the global register / global flag yet, but should be easy enough to add.

@mattbrailsford
Copy link
Collaborator Author

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?

@leekelleher leekelleher added this to the 0.11.0 milestone Sep 19, 2016
@JimBobSquarePants
Copy link

@mattbrailsford You should be able to check whether the result implements IProxy

@mattbrailsford
Copy link
Collaborator Author

mattbrailsford commented Sep 19, 2016 via email

@mattbrailsford mattbrailsford merged commit fa9ad41 into develop Sep 21, 2016
@mattbrailsford mattbrailsford deleted the wip/lazy-attribute branch September 21, 2016 08:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants