Implemented Processors as a replacement for Value Resolvers / Type Converters

[mattbrailsford]

PR for issue #142 and #144

[leekelleher]

Whoah, that's a lot to digest on a Friday night! 😉

I'll give some thought to this over the weekend, (I'm eager to dig into your code too).

Initial thoughts are with handling breaking changes - obviously we're intentionally v0.x, but still want to be considerate.

I'll feedback once I've had chance to review.

[mattbrailsford]

Yea, I did delete a load, but we can bring em back if needed :)

[leekelleher]

It was the Processor suffix on attributes that caught my eye.

[mattbrailsford]

Could change em, I just needed both for a period so added the "Processor" suffix to the new ones.

[leekelleher]

I'm wondering whether we should keep hold of the RegisterTypeConverter helper in facade.
(I found it simpler syntax - also that we do make a call to Umbraco's TryConvertTo in .GetConvertedValue 😉)

[mattbrailsford]

No problem

[leekelleher]

As discussed, these conversion checks could be made as processors and could be added to the end of the list of processors within .GetProcessedValue?

[leekelleher]

Overall, I'm happy with the direction that this is taking. I've left various inline comments, but those are quite trivial. I'm happy with the general "chainable processors" concept.

In terms of backward compatibly (as in not to totally piss off our users), what I'll do is let this PR mature a little, (as I'm sure you'll want to refine things), then I'll drop it in to one of my projects - see what pisses me off, then we can revert various classes with deprecated warnings/messages, (and base documentation updates on those findings too).

[JimBobSquarePants]

Noticed these have switched position throughout the solution. Is that deliberate?

[mattbrailsford]

I know it's a preference thing, but really there is very little evidence that having them inside the namespace declaration adds any benefit, and so given the default is to be outside the namespace, I think it'd make more sense to keep them outside.

[JimBobSquarePants]

Cripes @mattbrailsford you've been busy! Nice!

Overall I'm glad we are shifting from TypeConverters. Having both do the same functionality has never sat right with me and they cause issues with other things that use them. It's nice to see some much needed simplification of the codebase also.

I'm concerned at the apparent loss of our EnumerableInvocations methods though that ensure we can return single items and vice versa using a single processor. We'll need to plumb that in somewhere so all processors can take advantage of the functionality. (Prior to the PR only TypeConverters did I think).

We should also be very explicit with our naming. Some attributes have the Ditto prefix, some don't so we should make that consistent.

[mattbrailsford]

Hey @JimBobSquarePants thanks for the feeback. In regards to the loss of EnumerableInvocations methods, it wasn't intentional, I just thought all that code was to do with TypeConverters, but can easily add it back in / cut out the bits that should be kept. Strangely though, it would seem then that we don't have any unit tests around this code, as all tests passed with it removed so I guess it would be wise to add some in (I'll see if I can do this when I add it back in).

Re attribute naming, I've noticed the same and have started to standardize those too.

[mattbrailsford]

@leekelleher if you are happy with this and the merging of the processors and their attributes, are you happy for me to do all of this in the one branch (I've got them in separate branches atm)?

[leekelleher]

Yes, happy for them to be merged into a single branch. Just don't merge into 'develop' yet ;-) I'll sort out a v0.8.4 release today (if time permits) or tomorrow.

[mattbrailsford]

@leekelleher of course :)

[mattbrailsford]

Just a note on implementation, unfortunately we can't use generics with attributes, which we previously used to allow processors/resolvers to say what context/value types they expect, so instead I've added a DittoProcessorMetaDataAttribute that you add to your processor attribute declarations which says what types you expect (defaults to a value type of object, and context type of DittoProcessorContext)

[DittoProcessorMetaData(ContextType = typeof(MyProcessorContext))]
public class MyProcessorAttribute : DittoProcessorAttribute
{
    public override object ProcessValue()
    {
        return ((MyProcessorContext)Context).MyContextProperty;
    }
}

Because we don't use generics, it does mean you have to cast the value/context inside your processor, but I have added validation to the base processor to ensure the passed in types match those specified (though the value property is allowed to be null) so you can at least just cast them without having to do the type check yourself.

[mattbrailsford]

In the latest commit I've gone and converted everything left in the GetConvertedValue method into Processors and auto append them to the list of processors to run.

I've created one called EnumerableConverterAttribute to handle changing from / to enumerable, however I'm not sure if this should just be a general use processor, rather than one that runs by default? I have changed it a little so that it only has 1 responsibility which is to convert to / from enumerable, but the enumerable type is based on the Value type NOT the target property type as really the Processor should just have 1 function and it should pass through another processor if you need to do more work. This is what has made me wonder if it should just be generally accessible to add if you need it, but not on by default.

I'm happy to have the recursive ditto processor and try convert processors, but that one seemed like it might be better to suited to being added manually as needed?

[JimBobSquarePants]

Just to clarify with all these new attributes. Do I have to add them to each property or can I decorate the class with them?

Regarding the EnumerableConverterAttribute, the existing functionality allowed me to decorate any class I wanted with the DittoPicker converter and I would be assured that I would get the correct value back whether my property was an IEnumerable<T> or T. I could pick one item and still get a collection. That's why it targeted the property type. Picking one item should not require me to do any further mapping work.

[mattbrailsford]

@JimBobSquarePants I had previously made it so you had to do it at property level, but I've just gone ahead and allowed them at the class level as it didn't take much to make it work.

I've made it work like how type converter worked though, as in, the class level attributes get APPENDED to the end of a list of attributes found at the property level. Not sure if this is the right way or not.

RE the enumerables stuff, to be honest, I don't think it's that much different because your code won't trigger a conversion of the type itself if it's not already the right type by this point, so it'll just error, where as my code will let it through, but will likely error further on. I've just taken away that restrictiveness within the converter so that it has potential to be useful in other situations (such as further processing).

[mattbrailsford]

@JimBobSquarePants I'll just share this code snippet aswell to help explain why I made the enumerable converter like I did:

processorAttrs.AddRange(new DittoProcessorAttribute[]
{
    new HtmlStringAttribute(), 
    new EnumerableConverterAttribute(),
    new RecursiveDittoAttribute(),
    new TryConvertAttribute()
});

So these are the final processors that run, so assume we have a multi picker that returns IPublishedContent's (likely via a standard Umbraco value converter), but on your model, you have a single property of type MyModel. The way the EnumerableConverter works now would allow IEnumerable< IPublishedContent> to be converted to IPublishedContent allowing it then to go through the RecursiveDittoAttribute to do the conversion from IPublishedContent to MyModel. If I left it such that EnumerableConverter required the output type to be the final property type, then we would get an error, or we would need to make EnumerableConverter do more than just enumerable conversion.

Hope that helps

Matt

[JimBobSquarePants]

Yeah Matt that makes sense; It all looks like it's coming together now. PublishedContentExtensions in particular is much less spaghetti like than before your PR.

[mattbrailsford]

For sure, definitely much tidier. Amazing how one concept can just clean everything up :) Lee and I were talking the other day about how we didn't think it would change much more now, then Boom! :)

[leekelleher]

OK, we're good to merge this into the 'develop' branch, (needs rebasing first).

[leekelleher]