search

LionWeb-io / lionweb-csharp

Implements LionWeb specification in C#
Apache License 2.0
2 stars 0 forks source link

Deserializer is stateful i.e. can't be reused #18

Open dslmeinte opened 1 month ago

dslmeinte commented 1 month ago

You can't use a LionWeb.Core.M1.Deserializer instance more than once without getting weird behavior.

enikao commented 1 month ago

I'd argue that's by design: Classes are state + behavior, and we need to keep track of state for proper deserialization.

dslmeinte commented 1 month ago

It's only half by design: you instantiate a Deserializer with a set of languages, but then the intermediate state from during the deserialization is kept, wreaking havoc and confusion when you call Deserialize(serializationChunk) again (on a different chunk).

enikao commented 1 month ago

I agree, we could make the input serialization nodes / chunk also a constructor parameter. However, I think it's common to construct this class at a different place than using it.

In #11, I plan to change the deserializer API to IEnumerable<SerializedNode> anyway, to create a more "streaming" API. Then we should make sure it works when called several times with different SerializedNodes. I think this is a valid use case, e.g. I receive my whole model in chunks (because of transmission size limits), and want to reconstruct the whole model.

dslmeinte commented 1 month ago

That's a good idea. I would make it a 2-stage thing: construct a DeserializerFactory taking languages (and precomputing lookups) that can produce a stateful Deserializer instance that you can “feed” SerializationChunks consecutively.

JojoEffect commented 1 month ago

However, I think it's common to construct this class at a different place than using it.

I do not understand how this is an argument for having a stateful Deserializerthat can only be used once? If the code using the Deserializer is not aware of all information to recreate the state of the "already used" Deserializer and setup a new one, this looks to me like a not very convenient API.

Regarding convenience: Please keep in mind that a user of the LionWeb API does not want to multiple types/objects and recombination of those to "just" de-/serialize a bunch of nodes. I'm not sure what direction this discussion is going here or if this is about the user facing API at all...

dslmeinte commented 1 month ago

I think usage would look something look Deserializer.CreateNewFor(<languages>).Deserialize(<serializationChunk>)