Allow streaming interface to CBOR #3
Conversation
Instead of decoding directly CBOR to reflect.Value, pass the decoded CBOR values to an interface. The default implementation will recreate the reflect.Value objects and return them.
Unify error management in the DecodeValue innterface: allow all function to return errors. Rework some methods around arrays and maps to abstract more things to the backend. Add public MemoryValue type that allow other implementations of the DecodeValue interface to decode some values to memory (map keys for instance)
|
I am also working on go-ipld. These modifications are necessary for what I'm doing there (see ipld/go-ipld-deprecated#17). I haven't pushed any commit there yet but basically I need to be able to handle the CBOR structure as it is read from the underlying I believe these commit won't change much now. @whyrusleeping what do you think? |
|
@whyrusleeping could you please take a look here? or give me collab access? |
| _, err = io.ReadFull(dec.rin, dec.c) | ||
|
|
||
| if didread == 1 { | ||
| /* log.Printf("got one %x\n", dec.c[0]) */ |
|
lots of comments, this is a big changeset that i'm not convinced we need. What is the usecase? Are cbor documents going to be very large that holding them in memory is going to be an issue? Also, with a changeset like this, adding tests is going to be important. |
|
If you look at the changes, most of it is code that has been moved. Everything you commented on was code that was already there in another file. That's why I tried to split into multiple commits. The use case is to have a streaming interface where we process events instead. It can be used to avoid keeping more data than needed in memory, have the ability to process the data even if it hasn't been all received yet. The use case is also to be able to have detailed control over the generated data structure. In practice, this is used by ipld/go-ipld-deprecated#17 that implements a streaming interface to go-ipld. Even without the streaming interface, it could be used to customize the data structure. Generate |
|
Could you not split it up into separate files then? It makes it really difficult to review this when it looks like everything was removed and rewritten |
|
@whyrusleeping maybe try looking at the last two commits themselves? (FWIW, i think separating to many files is a very good idea. But, this code comes from elsewhere (maybe the readme should say so) and separating will make it much harder to merge any upstream changes. unless we want to forsake that option, which may be valid, i dont recall it being maintained. --- @whyrusleeping where was it from?) |
|
I'm fine with splitting it into separate files, but for the purposes of review, it makes things difficult. I guess I can just look at the last two commits |
|
Alright, If i can get a couple of tests to at least test the new behaviour (and maybe one to make sure the old behaviour still works) i'll merge this so i'm no longer blocking ipld stuff. |
|
The last commit should produce a diff that is a little easier to review. That's quite unfortunate that diff tools can't show that in any meaningful way. |
|
oh wow, that does look much better. good stuff! |
Allow streaming interface to CBOR
See #2 for rationale.