Adding archetype support to Ditto #44
Conversation
| @@ -32,6 +32,16 @@ | |||
| <WarningLevel>4</WarningLevel> | |||
| </PropertyGroup> | |||
| <ItemGroup> | |||
| <Reference Include="interfaces"> | |||
There was a problem hiding this comment.
Minimum version has to be v6 based as we are supporting v6 from now on.
|
That's a big commit! Cheers! 👍 I'll try and set aside some time tonight to have a look at it though I see @leekelleher get's up earlier than me on a Sunday so he'll probably beat me to it. 😄 |
| @@ -100,7 +100,9 @@ public override object ConvertFrom(ITypeDescriptorContext context, CultureInfo c | |||
| return multiNodeTreePicker; | |||
| } | |||
|
|
|||
| return base.ConvertFrom(context, culture, value); | |||
| // [ML] - return default list as its always nice to avoid null reference exceptions on lists =) | |||
There was a problem hiding this comment.
I dunno about this. You're might be right but Microsoft guidelines indicate always using base converters as a fallback. https://msdn.microsoft.com/en-us/library/ayybcxe5.aspx
There was a problem hiding this comment.
Cool, I think the base converters throw exceptions as values passed in are always strings from archetype, so some are unable to cast to the new type a flake out. Sure there will be some work around we can use.
There was a problem hiding this comment.
I have the same concern as @JimBobSquarePants - having the base converter as fallback.
@micklaw - Yup, I'm sure we can workaround issues in the body of the ConvertFrom.
|
Thanks for the pull request! I'll review in more detail tomorrow, (as I'm out & about today). To be upfront, I doubt we'll merge this in as is, as it ties Ditto too closely to Archetype. But I definitely think there should be a Ditto-like way for mapping Archetype data. Let's use this as a discussion point to move it forward. 👍 |
|
No worries, just for clarity, this amend doesn't have any dependency to archetype at all, so even if it's not in the solution this should still function alright. Sent from my iPhone
|
|
@micklaw Thanks again for your PR! I'm reviewing it now :-) Given the size of the commit, I'll probably ask a lot of questions... don't feel disheartened if I nitpick ;-) |
| /// <summary> | ||
| /// Defines if property is an Archetype | ||
| /// </summary> | ||
| public bool IsArchetype { get; set; } |
There was a problem hiding this comment.
I think this binds Ditto too much with Archetype (not in terms of assembly reference, but conceptually).
Should we consider other 3rd party property-editors with complex data? IsNuPicker, IsSirTrevor, IsVorto
It makes Ditto non-generic.
Let's look for an alternative solution - preferably using TypeConverters.
There was a problem hiding this comment.
@leekelleher Type converters makes sense to me, could potentially extend the ArchetypeModel (https://github.com/imulus/Archetype/blob/master/app/Umbraco/Umbraco.Archetype/Models/ArchetypeModel.cs) with a FieldSetAs method of something? Would these TypeConverters be completed in a separate assembly to avoid assembly dependency?
There was a problem hiding this comment.
👍 That's where my thinking is at the moment - separate assembly to specifically to handle Archetype data.
|
Ha, no worries, I'm thick skinned ;) At the GLUUG a few of the guys thought it was a good idea. I had already On 9 March 2015 at 10:30, Lee Kelleher notifications@github.com wrote:
Cheers Michael Law |
|
OK, hopefully I wasn't too much of an arsehole in my review? In concept, I am 100% sure that we should find a way to have Ditto-like way for mapping Archetype data! With the implementation, (as I keep going on about), I don't want the Ditto (core) to be aware of Archetype itself. I think there is a way by using TypeConverters - which I'd like us to explore. ❤️ Thanks again for the PR - much kudos to you! (For taking the time to get involved, it is appreciated!) I'll buy you a 🍺! |
|
@leekelleher @JimBobSquarePants Fellas, how would you feel if i refactored this code to be in the same wat as we discussed in a separate assembly and use a ArchetypeConverter to bind the data? Had a look over again and don't think it would much longer. Plus also sort the issues above that need amended obviously =) |
|
@micklaw - I think there is a definite need for Archetype POCO mapping. Discussing with @mattbrailsford about it being part of the Ditto project, (see my comment on #46), we came to the conclusion that it is beyond the scope of Ditto - in terms of both concept and ongoing support. Saying that, I don't want to discourage you (or anyone else) from developing a solution... so yes, please feel free to do it as a separate project. @mattbrailsford @JimBobSquarePants - did you want to add comment on this? |
|
@leekelleher Based on the previous pull request, I separated some of the logic to its own method for setting the value of the instance properties. I have taken this a step further and exposed the method in the class below as its own interfaced class with a single SetValue method. As i see it in my tests, this would be the only change to the ditto source to make this class and interface public. The rest can be hooked in a separate assembly via type converters. This would mean the archetype integration could be completely separate from the ditto source. Thinking about it if this method was made public, anyone could hook in to the Ditto model builder, so it would be possible to hook in Sir Trevor etc if anyone was interested. |
|
Here's a sample project separated out, didn't take long to hook up. Core has no dependency on archtypes or anything else. https://github.com/micklaw/umbraco-ditto/tree/archetype-refactor Hooked using type converters and classes hanging off the Archetype models =) |
|
Hey did you read this topic about Archetype? modelsbuilder/ModelsBuilder.Original#13 Not really related to this, but more people want to generate models for Archetype ;-). |
|
Hey, I defo think this has its place too. I'm a bit swamped for time at the On 7 May 2015 at 12:56, Jeroen Breuer notifications@github.com wrote:
Cheers Michael Law |
|
Closing this PR. Thanks again @micklaw - stellar effort here! Noticed your new project/repo at Ditto.Resolvers, I think that's the best move going forwards. (I may even contribute to it! 😎) |
|
No worries man, I think your spot on with the separate repository approach. I really like the new changes in the develop branch, defo makes the framework much more flexible =) Good work dudes. |
Adds support for archetype via the existing UmbracoPropertyAttributeClass which now contains an isArchetype field.
This hooks in to the existing TypeConverters and flow of application. Archetypes POCOs should follow the same naming convention standards as the main ditto POCOs, though will inherit from a base class of ArchetypeModel instead of the IPublishedContent.
I have ran this against almost all of the existing data types and conversion seems OK, though as archetype values are strings one or two data types may have slipped through.