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 KVArrayValue and KVBinaryBlob value types #93

Merged
merged 2 commits into from
Feb 16, 2024
Merged

Add KVArrayValue and KVBinaryBlob value types #93

merged 2 commits into from
Feb 16, 2024

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Feb 11, 2024

KVArrayValue is List<KVValue>, and KVBinaryBlob is byte[] because List<byte> would be silly.

@yaakov-h this is to be used by kv3 pls review. api surface generator is failing for some reason.

@yaakov-h
Copy link
Member

Could KVBinaryBlob be a Memory<byte> or at least ArraySegment<byte> so that callers can be more efficient than allocating exact-size managed arrays?

I'm also seeing a lot of overlap between KVArrayValue and KVCollectionValue, is there any way we could merge them, or is that more complexity?

@yaakov-h
Copy link
Member

The test failure seems to be a bug / unforeseen edge case in the test - a method has the same name as a method in the base class, but the base class has multiple methods by the same name.

@xPaw
Copy link
Member Author

xPaw commented Feb 11, 2024

I think most of the overlap is just unimplemented IConvertible region?

Could KVBinaryBlob be a Memory or at least ArraySegment so that callers can be more efficient than allocating exact-size managed arrays?

The caller is the deserializer, so it's gonna allocate the byte array regardless (but in binary deserialization this could be a memory view yeah -- although this has some implications on the ownership of the byte array)

@xPaw
Copy link
Member Author

xPaw commented Feb 15, 2024

I've fixed it to not crash when base type has multiple overloads. Maybe there's some library we could use that does correct api surface dumps for us?

@xPaw xPaw merged commit 4048410 into master Feb 16, 2024
6 checks passed
@xPaw xPaw deleted the kv3-types branch February 16, 2024 10:56
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