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 a facility allowing manual control of the decoding #91

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

damz
Copy link

@damz damz commented Aug 15, 2022

This PR adds a facility allowing full manual control of the decoding process.

While we are aware that the experimental deserializer interface exists, it is quite painful to use as it doesn't let the caller control the sequence of operations.

The implementation is centered around a Decoder object, which allows decoding ONE value at a particular offset.

The API for scalar values is straightforward:

DecodeBool() (bool, error)
DecodeString() (string, error)
DecodeBytes() ([]byte, error)
DecodeFloat32() (float32, error)
DecodeFloat64() (float64, error)
DecodeInt32() (int32, error)
DecodeUInt16() (uint16, error)
DecodeUInt32() (uint32, error)
DecodeUInt64() (uint64, error)
DecodeUInt128() (hi, lo uint64, err error)

For maps and slices, the API iterates over the items and provides a specific decoder for the value:

DecodeMap(cb func(key string, value *Decoder) error) error
DecodeSlice(cb func(value *Decoder) error) error

The implementation follows pointers automatically, and keeps track of the offset of the "next" value as soon as it is known, which facilitate the iteration of maps and slices. (For pointers, the "next" value is the value right after the pointer, for maps and slices it is the value right after the next item.)

@damz
Copy link
Author

damz commented Aug 31, 2022

@oschwald Any feedback on this after the discussion in #92?

@oschwald
Copy link
Owner

oschwald commented Sep 2, 2022

Sorry for taking so long on this. I still need to take a closer look. One thought I have is possibly moving the decoder to another package, but I have not looked into the implications of that.

@damz
Copy link
Author

damz commented Sep 9, 2022

I removed the patterns of using an error sentinel to stop the iteration and shadowing require in the tests, despite them being totally bog standard patterns, because your linter configuration is quite aggressive. (Even silencing these false positives is quite painful.)

It should pass the linter stage now.

@damz
Copy link
Author

damz commented Nov 7, 2022

@oschwald Do you have any feedback on this? We have this implementation in production now.

@andrei-dascalu
Copy link

This would be quite good to have on my side - any feedback ?

@oschwald
Copy link
Owner

Before considering merging this, I'd like to see the decoder moved to a separate package. Most users of this library are not likely to use this interface, and having it in the same package as the reader is likely to cause confusion. Once that is done, I think I would be willing to accept a PR as long as the documentation made it clear that the API was experimental and subject to change. I haven't had a need for this API myself and haven't yet had the opportunity to use it.

@damz
Copy link
Author

damz commented Mar 6, 2023

@oschwald The decoder relies on the internal decoder type right now, so it cannot be moved to a different package without significant refactoring.

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.

3 participants