Value converters for StringTemplate.Format()

[mlaily]

This PR introduces a StringTemplateValueConverterAttribute that can be applied on a property or field that is going to be formatted by one of the StringTemplate.Format() overloads taking an object as argument.

This attribute allows the value of the property or field it is applied on to be converted by a user-defined method prior to being formatted by StringTemplate.Format().

Why use this, and not a custom format provider?

• More convenient and lightweight
• Allows for different independent conversions to be performed on different properties, where a custom format provider would need to know about all required conversions (because only one format provider can be used at once)
• Conversion is only performed when the attribute is present (and not on all properties of a certain type. This could be implemented with a custom format string, but would be cumbersome)
• This method works well with format providers, so a converter could convert a value from one type to another, and the resulting value could be formatted according to a specific culture provided as a format provider
• Even though format providers could be abused to perform value conversion, they are for custom formatting, not conversion...

Example

The following class:

class Request
{
    [StringTemplateValueConverter(nameof(GetCustomRepresentation))]
    public DayOfWeek Day { get; set; }

    static string GetCustomRepresentation(DayOfWeek day) 
        => day.ToString().ToLowerInvariant();

    [StringTemplateValueConverter(nameof(ToCommaSeparatedValues))]
    public IReadOnlyCollection<int> Collection { get; set; }

    static string ToCommaSeparatedValues(IReadOnlyCollection<int> collection)
         => string.Join(",", collection);
}

Can be used like that:

var request = new Request
{
    Collection = new[] { 1, 2, 3 },
    Day = DayOfWeek.Friday
};
// https://coucou.com?ids=1,2,3&day=friday
var queryParams = StringTemplate.Format(
    "https://coucou.com?ids={Collection}&day={Day}", request);

[thomaslevesque]

Salut Melvyn !

I like the idea, but I think it would be better to implement converters as classes. Each converter would implement a IStringTemplateValueConverter interface, maybe something like this:

public interface IStringTemplateValueConverter
{
    bool CanConvert(object value);
    object Convert(object value);
}

And the attribute would refer to the convert type:

class Request
{
    [StringTemplateValueConverter(typeof(CustomDayOfWeekConverter))]
    public DayOfWeek Day { get; set; }

    [StringTemplateValueConverter(typeof(CommaSeparatedValuesConverter))]
    public IReadOnlyCollection<int> Collection { get; set; }
}

public class CommaSeparatedValueConverter : IStringTemplateValueConverter
{
    public bool CanConvert(object value) => value is IEnumerable<object>);
    public object Convert(object value) => string.Join(",", ((IEnumerable)value).Cast<object>());
}

public class CustomDayOfWeekConverter : IStringTemplateValueConverter
{
    public bool CanConvert(object value) => value is DayOfWeek);
    public object Convert(object value) => ((DayOfWeek)value).ToString().ToLowerInvariant();
}

Pros:

• less code in the "model" class
• more easily reusable converters
• less reflection code => better performance
• assuming converters are stateless, they can be cached and reused => better performance
  Cons:
• a bit more boilerplate code. This could be improved by introducing a generic base class to avoid casts and explicit typechecks (e.g. StringTemplateValueConverter<T>)

[mlaily]

Hey!

Thanks for your comment.

I agree converter classes feel cleaner, but I wasn't sure it would be worth the effort. I liked the simpleness of NUnit test case sources...

I'm a bit sad to lose the convenience and freedom allowed by Expression.Call() about converter method definitions.
A converter interface with object input and output seems like a set back, even with a generic base class on top.

Anyway, I'll see what I can do.

I'm not sure about the way to go for caching though.
I managed to re-use the existing getters cache mechanism, but I doubt I can make it work with converter instances.

Should I create another cache instance just for the converters?

[thomaslevesque]

Should I create another cache instance just for the converters?

Yes, I think it would be best

Thanks !

[mlaily]

I finally got the time to do a proper implementation with class converters! :)

Things to note:

CanConvert takes a Type and not an object ( I mention this because it is different from your suggestion, but I think it makes more sense)

There is a slight issue (that might just not be one...) with the current implementation and caching: IStringTemplateValueConverter instances are cached both in a dedicated cache, and also in the getters cache (because they are a part of the compiled getter expressions).
This means that if the IStringTemplateValueConverter cache is cleared but not the getters cache, converter instances will only be reused by already cached getters, and a new instance of IStringTemplateValueConverter will be created if necessary when accessing a property that does not have a cached getter yet. (if the IStringTemplateValueConverter cache was populated, the current proper IStringTemplateValueConverter would be used instead)

This worries me a bit, BUT it's probably not that important, because all the caches are currently cleared together. This might be a problem if this changes though...

EDIT: I added a unit test for that. It's not foolproof but it's a start (it can only check ClearCache() does the right thing, but it won't detect a problem if the caches are cleared from another place).

What do you think?

[thomaslevesque]

CanConvert takes a Type and not an object ( I mention this because it is different from your suggestion, but I think it makes more sense)

Fine by me!

IStringTemplateValueConverter instances are cached both in a dedicated cache, and also in the getters cache (because they are a part of the compiled getter expressions).

Oh, I didn't think of that.

This worries me a bit, BUT it's probably not that important, because all the caches are currently cleared together. This might be a problem if this changes though...

I don't think it's a problem. I don't intend to expose a way to clear caches selectively. In fact, I'd rather not have the ClearCache method at all, but I put it there anyway in case the cache grows too much.

Now I'll do a proper review ;)

[thomaslevesque]

This is pretty good! I have a few comments, but they're quite minor.
Thanks !

[mlaily]

Just checking in: is something blocking this PR, or did you simply not have the time to merge it yet?

[thomaslevesque]

Just checking in: is something blocking this PR, or did you simply not have the time to merge it yet?

Ah, I didn't get a notification for your last push... I'll merge it now. Thanks @mlaily!

[mlaily]

Thanks a lot! :)

Any estimated time frame for a new release? (Not that it's pressing or anything)