YamlDotNet icon indicating copy to clipboard operation
YamlDotNet copied to clipboard

IYamlTypeConverter.readYAML is too limited?

Open matthijskooijman opened this issue 9 years ago • 10 comments

I've been working on a custom deserializer, by implementing the INodeDeserializer interface. Its Deserialize method has access to the EventReader object, allowing the use of e.g. reader.Expect() and it has access to a nestedObjectDeserializer, which can be used to deserialize parts of my object using the default rules (I'm deserializing a custom vector class, which is written as a list of two integers).

When adding custom serialization for the same class, I found that that needed a custom IYamlTypeConverter implementation. That interface seems to handle deserialization as well, so it seems it is easy to implement both serialization and deserialization in the same class. However, its readYaml() method only gets passed the IParser and Type objects, which (AFAICS) make it a lot more tricky to actually parse a more complicated object. Perhaps this interface was intended for converting simple scalar values, but it seems a missed opportunity that it does not get the same values as Deserialize() so it is way more powerful.

When I started writing this report, I believed that his would prevent me from properly deserializing (because the type converter does not have a way to only handle serialization, so it would prevent my custom INodeDeserializer from running), but I just realized that I can just not register the type converter when doing deserialization, which should allow me to use the INodeDeserializer for deserialization and the IYamlTypeConverter for serialization.

Still, it would be nice to have a more symmetrical way to do this. Of course, I might be misunderstanding the purpose of these interfaces and there might be a better way - I'm mostly guessing around based on looking around the sources, of course :-)

matthijskooijman avatar Mar 04 '16 16:03 matthijskooijman

It seems something similar holds for writeYaml(), which only has access to the IEmitter, not the slightly more high-level IEventEmitter.

matthijskooijman avatar Mar 04 '16 17:03 matthijskooijman

Hi, Your comments make a lot of sense, I agree that the interfaces are too limited. That's probably due to not needing myself to implement them, and lack of feedback from other users. Let's fix that. Would you like to submit a pull request with your suggested changes?

aaubry avatar Mar 07 '16 14:03 aaubry

Yeah, making an API for hypothetical usecases is always tricky. As for a pullrequest - my project is on a bit of a tight schedule, so I will probably have to just work around the problem if possible. I might look at a proper solution, though.

What's your take on backward compatibility? Changing IYamlTypeConverter will of course break existing programs that make use of it?

matthijskooijman avatar Mar 13 '16 14:03 matthijskooijman

A possible fix for this issue is in #203. This eliminates EventReader, and equivalent functionality becomes available as extension methods for IParser. This removes the design flaws of EventReader, and when you have an IParser, you don't need to wrap it inside another object to be able to use it effectively.

aaubry avatar Aug 30 '16 23:08 aaubry

I could be missing something but I believe this is still an issue. I've spent a day or so trying to figure out how to properly support my custom type serialization/deserialization, including looking through the code a fair amount, and I can't find a way that's both symmetrical and properly nests. If I use IYamlTypeConverter, I can't find a way to dispatch to a nestedObjectSerializer/Deserializer, which means I seem to need to parse the whole object hierarchy in a single IYamlTypeConverter. What I really want is to define/register an IYamlTypeConverter for each custom type and but still be able to invoke the framework to handle nesting or built-in types.

I notice that I CAN do this on the deserialization side if I use INodeDeserializer because Deserialize has a nestedObjectDeserializer but that doesn't cover the serialization side (and, maybe I'm being dense, but I can't find anything like an INodeSerializer interface). I also seem to be able to do it if each of my types implements IYamlConvertible but I strongly, strongly dislike coupling my types to any specific serialization framework (and this also requires me to make my types mutable, which I was hoping to avoid).

I think this can be resolved by adding a nestedObjectDeserializer to IYamlTypeConverter.Read and a nestedObjectSerializer to IYamlTypeConverter.Write. If I'm missing something, please let me know.

Epp-code avatar Jun 20 '19 00:06 Epp-code

You are correct that this is a limitation of IYamlTypeConverter. A possible workaround is to pass it an instance of ISerializer, but I agree that this is not ideal. The interface should be extended to be like IYamlConvertible, which receive delegates to invoke the serializer / deserializer. This should be simple to implement, although it requires a breaking change.

aaubry avatar Jun 20 '19 17:06 aaubry

Hi!

Thanks for all the effort you have put in your library. I do share the same issue as expressed by the OP. Currently it appears to be way too cumbersome to implement ITypeConverter, even just for simple scalar values. In some cases it feels like having to write a nearly complete serializer/deserializer. For my use case I wanted to apply the hyphenated naming convention to enum values, which by the way could be supported by YamlDotNet ootb, but that's another issue. For deserialization I was expecting that I would get the hyphenated string, convert it to pascal casing and pass that along to something, anything. Instead, and if I'm not mistaken, it requires implementing a visitor interface with 11 methods, 10 of which would just throw NotSupportedException because they're irrelevant for that case 😆

For the moment, I'm going to have to reluctantly implement IYamlConvertible on my classes, because that appears to be currently the only simple way to customize the serialization/deserialization processes. But I'll be looking forward to the evolution of IYamlTypeConverter so that my classes can remain "POCO" (I'd rather avoid stacking decorations on them for every library that will process them).

antoinebj avatar Aug 03 '19 12:08 antoinebj

@antoinebj While it is true that the interface could be improved, it seems suitable to your use case. There's no need to implement any visitor. This is how I would do it:

public class ApplyNamingConventionToEnums : IYamlTypeConverter
{
    private readonly INamingConvention namingConvention;

    public ApplyNamingConventionToEnums(INamingConvention namingConvention)
    {
        this.namingConvention = namingConvention;
    }

    public bool Accepts(Type type)
    {
        return type.IsEnum;
    }
    
    public object ReadYaml(IParser parser, Type type)
    {
        var scalar = parser.Expect<Scalar>();
        
        // Naming conventions are not bidirectional, so we need to test each value.
        // This is unfortunate but this implementation could be improved.
        
        var names = Enum.GetNames(type);
        var valueAsString = names.First(n => namingConvention.Apply(n).Equals(scalar.Value));
        return Enum.Parse(type, valueAsString);
    }
    
    public void WriteYaml(IEmitter emitter, object value, Type type)
    {
        var valueAsString = namingConvention.Apply(value.ToString());
        emitter.Emit(new Scalar(valueAsString));
    }
}

Check a working example that uses this code: https://dotnetfiddle.net/JyyiVV

aaubry avatar Aug 03 '19 15:08 aaubry

@aaubry This is great, thank you very much! I had missed the Expect() method

antoinebj avatar Aug 03 '19 18:08 antoinebj

Are there any chances that that IYamlTypeConverter will be updated in the nearest future?

valdisz avatar May 07 '21 09:05 valdisz

This issue appears to be abandoned, I'm going to close it.

EdwardCooke avatar Jan 13 '23 20:01 EdwardCooke