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

Add KVDocument as a root type #94

Merged
merged 3 commits into from
Feb 16, 2024
Merged

Add KVDocument as a root type #94

merged 3 commits into from
Feb 16, 2024

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Feb 11, 2024

I made it extend KVObject for backwards compatibility.

KV3 requires a special root object because it has a header that contains format and encoding data. KV2 might have some extra stuff too.

Perhaps there's a better way of doing this?

    public class KVHeader
    {
        public Guid Encoding { get; set; }
        public Guid Format { get; set; }
    }

I think Serialize should have a new overload added to accept this, rather than changing the existing one because this would be a compat break. But how would this work with Serialize<TData>?

@yaakov-h
Copy link
Member

I would avoid using "File" to refer to something that can exist without ever touching disk. Other libraries have gone the route of "Document".

I don't see an obvious "better way".

I would expect that for Serialize() this encoding data should be provided through the KVSerializerOptions object? Ideally with well-known constants/values rather than BYO-GUID (though that should also be allowed).

@xPaw
Copy link
Member Author

xPaw commented Feb 11, 2024

KVDocument sounds fine.

Ideally with well-known constants/values rather than BYO-GUID (though that should also be allowed).

This is covered in the kv3 pr: https://github.com/ValveResourceFormat/ValveKeyValue/blob/3a0c305f14cccc19860a64c93ee76da245586099/ValveKeyValue/ValveKeyValue/KeyValues3/Encoding.cs

For serializing text files, you wouldn't really need to provide these (default to text encoding in generic format).

I'm not sure about adding them to KVSerializerOptions because deserialize gives you a KVDocument that contains these, would make sense for serialize to also take this, which works for generic kv objects, but not the typed variant.

EDIT: KV3 will add KVHeader class, so that could be passed into the serializer options, or as an optional argument.

@xPaw xPaw changed the title Add KVFile as a root type Add KVDocument as a root type Feb 11, 2024
@xPaw xPaw changed the title Add KVDocument as a root type Add KVDocument as a root type Feb 16, 2024
@xPaw xPaw merged commit bb29fd0 into master Feb 16, 2024
6 checks passed
@xPaw xPaw deleted the kvfile branch February 16, 2024 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants