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

TypeConverter for serializing and deserializing ValueOf types #3

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

meinsiedler
Copy link
Contributor

Hi!

This is my second PR here in this repository.

I added a TypeConverter for ValueOf types. This is needed, if you want to serialize and deserialize types, e.g. with Newtonsoft.Json. The serializer needs a hint how to serialize and deserialize ValueOf types correctly. (e.g. a string is correctly deserialized to a ClientId).

The generic ValueOfTypeConverter can simply be used as the following. (See the test cases for details):

[TypeConverter(typeof(ValueOfTypeConverter<string, ClientId>))]
public class ClientId : ValueOf<string, ClientId> { }

Note that this feature adds a dependency to System.ComponentModel.TypeConverter.

Please let me know what you think.

Cheers!

@sebgod
Copy link

sebgod commented Sep 4, 2019

@mcintyre321 this seems to be a useful addition, and seeing you posted something similar as a fiddle https://dotnetfiddle.net/Ec7IKN, is there anything in plan to add it to the library?

@mcintyre321
Copy link
Owner

This is a really neat and useful pull request, but the reasons I couldn't decide if to merge this or not are:

  1. I haven't investigated the impact of adding System.ComponentModel.TypeConverter as dependency (especially WRT compatibility with all the different platforms, from Core/FX/Mono etc. etc.) - it's particularly important for a utility library

  2. It's 'opt-in', opinionated behaviour, i.e. the developer has to decide to mark their class up with [TypeConverter(typeof(ValueOfTypeConverter<string, ClientId>))] (rather than it being on the ValueOf base class), so there's no absolute requirement for this code to live in the main ValueOf project (it could equally be in a ValueOf.TypeConverter package. There is perhaps convenience though (a second dll for such a tiny class seems pointless).

If 1. could be resoved to my satisfaction (i.e. is System.ComponentModel.TypeConverter available everywhere , including Blazor, which it probably is, I wouldn't mind merging).

My fiddle requires JSON.NET, so the reasons to not include the ValueTupleContractResolver and dll dependency in the main package are even stronger.

@sebgod
Copy link

sebgod commented Sep 4, 2019

Thanks @mcintyre321 , that explains it very well.
Just out of curiosity, would the TypeConverter annotation also work on the ValueOf class itself?

There is unfortunately no license attached to the fiddle, could you clarify on that part? Ideally this code would be nice to have in a separate NuGet package.

@tiagor87
Copy link

I think this feature should be added as an extension, maybe some sort of Serializer configuration and not as an annotation on class.

@SteveDunn
Copy link

I don't want to steal people away from this very useful library, but you might want to take a look Vogen as it does exactly these things. It has (opt in) conversions for Newtonsoft.Json, System.Text.Json, TypeConverter, Dapper, and EFCore

It's somewhat different to this library as it enforces much stricter checking of validity, e.g. it won't let you default a value object struct, or new a value object class:

https://github.com/SteveDunn/Vogen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants