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

Error handling #120

Open
mhasel opened this issue Sep 25, 2023 · 2 comments
Open

Error handling #120

mhasel opened this issue Sep 25, 2023 · 2 comments
Assignees

Comments

@mhasel
Copy link

mhasel commented Sep 25, 2023

Hi!

First of all, thanks for your work on this project. I've recently started using this crate for XML validation and when running tests with syntactically incorrect XML files (which might be the result from bad merges) I noticed that internal errors from libxml2 result in a panic.

I'm happy to contribute on this matter myself, I was just wondering which route you might want to take here. I've had a look at all the TODOs regarding error handling - turning the null-pointer checks in
https://github.com/KWARC/rust-libxml/blob/da4369a035557667650a221bc89d5e59d0efb247/src/schemas/parser.rs#L24C1-L26C6
from panics to results would necessitate either:

  • making breaking API changes due to the changed return-type of the function
  • to deprecate these functions and introduce new versions that support error handling

All the other occurrences of panics due to missing error-handling already return a Result<Self, Vec<StructuredError>>, so implementing these should only be a matter of creating new StructuredErrors, i.e. StructuredError::null_ptr() and StructuredError::internal().

Any and all feedback is appreciated!

@dginev
Copy link
Member

dginev commented Sep 25, 2023

@mhasel thank you for looking into this.

As you may suspect, the panics in question were added there to forcefully remind us we should be fleshing out an error interface at some point. Maybe your newly opened issue is that occasion, and especially if you are willing to do a contribution yourself - it would be welcome!

Your plan for creating more StructuredError variants sounds workable, though whether you need the wrapping Vec or not is up to you for the final Result<Self, StructuredError> type here.

Note to self: This will likely move us to a version 0.4.0 to signal we've broken an interface, and maybe that is a good occasion to double-check if any of the other issues can benefit of getting batched in.

@mhasel
Copy link
Author

mhasel commented Oct 2, 2023

Hey, sorry for the radio silence. Been under the weather these past couple of days. I should have a PR ready soon, just need to sort out tests and the error-domain codes.

mhasel added a commit to mhasel/rust-libxml that referenced this issue Oct 3, 2023
change panics into recoverable results

Refs: KWARC#120
mhasel added a commit to mhasel/rust-libxml that referenced this issue Oct 3, 2023
change panics into recoverable results

Refs: KWARC#120
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants