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

Make Methods in Serializers virtual so they can be overwritten in der… #33

Closed
wants to merge 1 commit into from

Conversation

jogibear9988
Copy link

…ived classes

@jogibear9988
Copy link
Author

And this one? is it ok?

@tomba
Copy link
Owner

tomba commented Nov 15, 2015

I've been thinking about this, but I'm not too enthusiastic about making the methods virtual. In fact, the classes should be sealed, but somehow I missed adding that.

What you're really interested in is the GenerateWriterMethod/GenerateReaderMethod in GenericSerializer, right? We could make public static functions for those, which you could then call from your custom serializer.

@jogibear9988
Copy link
Author

No, im interested in the Handles method of the generic serializer, cause my types don't have the serializable attribute!

@tomba
Copy link
Owner

tomba commented Nov 16, 2015

What I mean is that if the methods I mentioned were public, you could easily implement a custom serializer that would call those methods, right?

As I said, I'm not enthusiastic about making the classes inheritable. And especially as the only use case so far has been your issue, serializing classes that are not marked as Serializable. That's not a proper use case, as it says that the classes weren't designed to be serialized and thus they really shouldn't be serialized.

I'm not saying you shouldn't serialize them, and maybe they work fine for you. But I'm saying that I don't want to change NetSerializer too much to accommodate such a hack use case.

@jogibear9988
Copy link
Author

I understand! But I don't see what the problem could be if you make them virtual? I could also copy the Code of GenericSerializer, but for that a few of the Methods of your serializer need to be public visible!

@tomba
Copy link
Owner

tomba commented Nov 16, 2015

The problem with making classes inheritable (and making methods virtual) is that you should design the classes for that use. Just blindly changing methods virtual may easily create problems. Now, could be that the classes here are simple and making them virtual is not a big deal. But again, I'd rather not go that way just to support your problem case.

It is ok to make some methods public in the serializer. It should be possible for the user to create custom serializers that do the same work than the built-in serializers. So it should be possible to copy the built-in serializers and have them as custom serializers.

I haven't tested custom serializers much, so I'm sure there are private methods that should be made public.

@tomba
Copy link
Owner

tomba commented Nov 16, 2015

I was looking a bit at this problem, and I still have no solution. I think it's clear that the user should be able to write custom serializers that would be as good as the current built-in ones. But at the same time I'm not ready to make all the methods and classes public which are currently needed to do that, as then the public API would grow a lot, with rather low level methods which I'd then need to support in later NetSerializer versions.

@jogibear9988
Copy link
Author

No problem, if you won't change, I will create my own Nuget Package from my fork!

@tomba
Copy link
Owner

tomba commented Nov 17, 2015

That's probably best for the time being. I'll continue working on this when I have time. I want to keep the public API clean, so this needs some thought on how to implement.

I've created an issue for this work: #39

@tomba
Copy link
Owner

tomba commented Feb 5, 2016

Closing this. There's issue #39 for tracking the work needed.

@tomba tomba closed this Feb 5, 2016
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.

2 participants