y-crdt icon indicating copy to clipboard operation
y-crdt copied to clipboard

add `Result` for `Encode` trait

Open darkskygit opened this issue 2 years ago • 3 comments

fix #288

this pr aims to add Result<(), lib0::error::Error> return value to the encode function of Encode trait

this allows developers to safely handle errors when encountering erroneous data

after this pr is merged, I plan to modify the panic in yrs to a YrsError structure generated by thiserror in the next pr, which can make the error message clearer

related issues: #272

at present, the related calls of yffi/ywasm directly unwrap the encode result. that can keep cause thread panic like the original internal panic. we can improve the related apis to allow catch errors in the future.

darkskygit avatar Mar 29 '23 14:03 darkskygit

i have added the Result return value to more apis here: https://github.com/toeverything/y-crdt/commit/1dc82f202450342738940c687de06358da7fdcdc

but the test case needs to add a lot of unwrap: https://github.com/toeverything/y-crdt/commit/e278bb63252d172729d24ca418566e18e0cf72f4

i'm worried about whether this will cause review pressure, if you think this will not be a problem, I will cherry pick these commits into this pr

darkskygit avatar Mar 30 '23 09:03 darkskygit

@darkskygit thanks for your contribution. I'll check it in more detail, once I return from the vacation.

While I was in favour of using results and std::io::Write-based encoding once I joined a project, we eventually decided to no go this way. It wasn't an accidental decision. It may be a good time to reevaluate it and make changes, however please keep in mind that doing this kind of PR without prior proposal may turn into a wasted effort from your side.

Horusiath avatar Mar 30 '23 17:03 Horusiath

@darkskygit thanks for your contribution. I'll check it in more detail, once I return from the vacation.

While I was in favour of using results and std::io::Write-based encoding once I joined a project, we eventually decided to no go this way. It wasn't an accidental decision. It may be a good time to reevaluate it and make changes, however please keep in mind that doing this kind of PR without prior proposal may turn into a wasted effort from your side.

Thank you for your reply, if you have any suggestions for revision, please feel free to let me know, I will make revisions as soon as possible.

darkskygit avatar Apr 05 '23 05:04 darkskygit