[thrift-remodel] Rework thrift reader API#8341
Conversation
| /// Read a Thrift encoded [list] from the input protocol object. | ||
| /// | ||
| /// [list]: https://github.com/apache/thrift/blob/master/doc/specs/thrift-compact-protocol.md#list-and-set | ||
| pub(crate) fn read_thrift_vec<'a, T, R>(prot: &mut R) -> Result<Vec<T>> |
There was a problem hiding this comment.
This is a big reason why this change was needed...trying to get the reading of vectors of serializable objects working with the changes to the ThriftCompactInputProtocol changes was just too hard. Adding this function a) worked and b) made implementing the thrift macros easier.
mbrobbel
added
api-change
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Looks good to me -- I don't fully appreciate why using a special trait rather than TryFrom makes things easier, but on the other hand this code is simpler and more explicit so I give it a major 👍
| } | ||
| } | ||
|
|
||
| /// A high performance Thrift reader that reads from a slice of bytes. |
|
Thanks @alamb!
TBH neither do I 🤣. It was mostly down to the Anyway, onward! |
3 participants
Which issue does this PR close?
Note: this targets a feature branch, not main
Rationale for this change
As I started on decoding thrift page headers, I found that the way I had been going was no longer going to work. This PR begins the process of abstracting the thrift reader to allow for other implementations.
What changes are included in this PR?
In addition to reworking the reader itself, this PR moves away from the previous
TryFromapproach and instead adds aReadThrifttrait.Are these changes tested?
Should be covered by existing tests
Are there any user-facing changes?
Yes