Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IYamlTypeConverter.readYAML is too limited? #168

Closed
matthijskooijman opened this issue Mar 4, 2016 · 11 comments
Closed

IYamlTypeConverter.readYAML is too limited? #168

matthijskooijman opened this issue Mar 4, 2016 · 11 comments

Comments

@matthijskooijman
Copy link

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
Copy link
Author

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

@aaubry
Copy link
Owner

aaubry commented Mar 7, 2016

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?

@matthijskooijman
Copy link
Author

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?

@aaubry
Copy link
Owner

aaubry commented Aug 30, 2016

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.

@Epp-code
Copy link

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.

@aaubry
Copy link
Owner

aaubry commented Jun 20, 2019

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.

@antoinebj
Copy link

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).

@aaubry
Copy link
Owner

aaubry commented Aug 3, 2019

@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

@antoinebj
Copy link

antoinebj commented Aug 3, 2019

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

@valdisz
Copy link

valdisz commented May 7, 2021

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

@EdwardCooke
Copy link
Collaborator

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

@EdwardCooke EdwardCooke closed this as not planned Won't fix, can't repro, duplicate, stale Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants