Add PropertyValueType attribute to help models builder #49

[cwjackso]

No description provided.

[mattbrailsford]

Guess this means single item mode won't work properly then as our resolver can resolve multiple value types.

[leekelleher]

@zpqrtbnk Can you offer any advice on how we could handle multiple value types?

e.g. our ValueConverter could return either an IEnumerable<IPublishedContent> or a single IPublishedContent

[cwjackso]

I imagine two property value converters would be needed. However the "IsConverter" method has no context of value so it could not be done at run time.

I suppose it opens the discussion into if a single property editor should be returning two different types though. Its easy in code to .FirstOrDefault() if you know there is only going to be one. but that's a breaking change now.

[mattbrailsford]

Yea. Guess it just sucks as these are new rules that weren't a requirement previously. Yes we could just return enumerable, but would make the code a bit uglier. Don't think it'll be possible to have converters for the same PE though.

[cwjackso]

@mattbrailsford it may be possible just looking at the converter actually.

the single return works on min/max item pre-values so splitting the converter in two should be possible. I have 30 mins if you want me to look at another pull request?

[mattbrailsford]

Who am I to stop you :)

[leekelleher]

@cwjackso By all means, please do 😎

[cwjackso]

Ok, that works nicely! I've pulled some logic out into static extensions but thought that would be cleaner than extending the converter?

Thanks

[mattbrailsford]

Have you tested it in non-models builder manner? ie, from within a normal view can you still do Model.GetPropertyValue< IEnumerable< IPublishedContent>>("alias") and Model.GetPropertyValue< IPublishedContent>("alias")

[leekelleher]

@mattbrailsford I think they should be okay, as GetPropertyValue extension method is a wrapper for GetProperty("alias").Value.TryConvertTo<T> (obviously that's "in a nutshell" 😉) ... so the PVC returns the appropriate object-type and TryConvertTo<T> attempts to cast/convert it accordingly.

(Sorry if I'm teaching you to suck eggs 😊)

[leekelleher]

@cwjackso Good work, we'll review as soon as we can.

[mattbrailsford]

It's cool, I'm just throwing things out there as they come to mind, not really with any backing :)

[mattbrailsford]

Nice work @cwjackso though on such a fast fix

[cwjackso]

Like a fool I only tested the model building and hadn't tested the retrieval at all. I wasn't returning the converted value in either converter!

The last commit is now tested for retrieval via both strong typed model and .GetPrpertyValue(). As @leekelleher says they are essentially the same thing though.

Thanks

[mattbrailsford]

Never hurts to double check :) Nice work fella 👏