Provide a nicer way to populate calculated properties

[mattbrailsford]

Right now, if you have a value that needs to be populated from multiple fields within an IPublishedContent we have 2 options. One is to pass in a func to the .As call, and the other is to register an event handler and detect that type being converted.

The problem with the first option is that it requires you to be in control of the .As call, which may not be the case (such as in DitFlo), and the problem with the event handler is that it moves the logic of populating the calculated values away into a "hidden" place, ie, it's not very discoverable.

Whilst looking into how JSON.NET handles similar things, I came across the OnDesrializedAttribute and thought, what if we added a [DittoOnConverted] attribute to allow people to populate these calculated properties more easily.

Now, I have two possible implementations of this attribute, on a method or on the class (or both).

On a method

public class CalculatedModel
{
    public string Name { get; set; }

    [DittoIgnore]
    public string AltText { get; set; }

    [DittoOnConverted]
    internal void OnConverted(ConvertedTypeEventArgs e)
    {
        AltText = e.Content.GetPropertyValue("siteName", true) + " " +
            e.Content.GetPropertyValue("siteDescription", true);
    }
}

This would be the same as the OnDeserializedAttribute referenced above. With this, we define one or more methods to call on the entity after the entity is converted. The plus side of this is that the logic for populating the calculated properties is kept with the model itself. The downside is, it means you now have logic inside you POCO's..

On the class

[DittoOnConverted(typeof(CalculatedModelOnConvertedHandler))]
public class CalculatedModel
{
    public string Name { get; set; }

    [DittoIgnore]
    public string AltText { get; set; }
}

public class CalculatedModelOnConvertedHandler : DittoOnConvertedHandler<CalculatedModel>
{
    public void OnConverted(ConvertedTypeEventArgs e){
        Model.AltText = e.Content.GetPropertyValue("siteName", true) + " " +
            e.Content.GetPropertyValue("siteDescription", true);
    }
}

When registered on the class, we could have it reference a Type for a DittoOnConvertedHandler which can just have a single function to populate the model variables. The pro of this is that it keeps your poco's pocos, but it's a bit more work to setup and requires extra classes to be setup.

Now, we could also just implement both so people have options as we could just detect it on a method or on a class and have it trigger the different options based upon it's usage.

What do people think?

[mattbrailsford]

Dunno if this is crossover or not, but in commit 72250be I've made DittoValueResolver non-abstract so it's easier to make simpler resolvers (see the test for an example).

I guess it doesn't hurt to have options, but both solutions give ways to set calculated values, the OnConvertedAttribute just gives a nice way to do a bunch of resolutions if making individual value resolvers would be too much work (ie, if the resolver is only needed by one object type, it's a lot of code for little gain).

In this case, I think ValueResolvers should be used for repeated resolutions, and these OnConverted handlers can be used for one offs.

Thoughts?

[mattbrailsford]

Additional question, should we add an [OnConvertingAttribute] option that runs before property mapping? (that's a bit trickier as the type is created mid way into the conversion).

[JimBobSquarePants]

I'm a big fan of this; the easier to bind to the events the better. I think perhaps the second option of a second class feels neater to me.

In this case, I think ValueResolvers should be used for repeated resolutions, and these OnConverted handlers can be used for one offs.

Do you mean by this that the ValueResolvers should be static?

I think we'd definitely have to implement [OnConvertingAttribute] if only for API consistency so we shall have to be careful to ensure that whatever we add does allow both events to be handled easily.

[mattbrailsford]

Yea, me and Lee are discussing this at the moment and are half tempted to remove the event handlers version all together as it moves code into hidden places, where as this is much more declarative and obvious when you look at the model as to what is going on.

I agree with adding the OnConverting option too.

I think I'm going to change the class level attribute to be more [DittoConversionHandler()] and have the handler base class have an OnConverting and an OnConverted method so that they can both be handled from the same event handler, without the need of multiple attributes on the class.

I still like the [DittoOnConverted] attribute though for those that like to keep all the logic together (think of it like a constructor populating it's properties) so may just keep both as options.

What do you think about loosing event handlers all together in favour of this attribute driven alternative?

[mattbrailsford]

I've gone ahead and made these changes then. If you look in the code however, I've moved the firing of these specific handlers to within the GetTypedProperty method just before and just after properties are populated.

This is fine for these attributes, but not sure it means the other events we currently support should also fire at this level? Worth noting, these new Converting handlers are not cancelable like the current events are, they are purely to be used as triggers.

[JimBobSquarePants]

I'll have a good look at the code at lunchtime but yeah, I agree, remove the overloads and move to attributes. Much more obvious to developers working within a team then as to what is going on.

[mattbrailsford]

In my last commit 4e91968 I've done some pretty big updates so you'll have to let me know your thoughts.

I've changed all the callback functions you pass to .As to use the new ConversionHandlerContext object, rather than the EventArgs we had, and I've removed the events stuff all together. I figured I'd keep the callback functions as these are a nice way to do one off updates (ie, in one conversion instance, you might want to convert it differently) however I've dropped the events alltogether as there is nothing you can do in the events that can't be done via the new attributes and the attributes are much more explicit.

Let me know what you think

[mattbrailsford]

Oh, I also renamed GetTypedProperty to ConvertContent as it seemed more logical

[mattbrailsford]

Ok, well, I think the general consensus is that this is a nicer way to go than events, so I'm just gonna merge it in and we can make tweaks in the develop branch if anyone doesn't quit like the implementation.

[JimBobSquarePants]

Late to the party. All is good.