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 Result for Encode trait #286

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

darkskygit
Copy link

@darkskygit darkskygit commented Mar 29, 2023

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 darkskygit changed the title add Result for Encode trait add Result for Encode trait Mar 29, 2023
@darkskygit
Copy link
Author

i have added the Result return value to more apis here: toeverything@1dc82f2

but the test case needs to add a lot of unwrap: toeverything@e278bb6

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

@Horusiath
Copy link
Collaborator

@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.

@darkskygit
Copy link
Author

@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.

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.

api proposal: add Result return value to Encode
2 participants