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

Redefine Errors interface #124

Open
torymur opened this issue Dec 12, 2024 · 5 comments
Open

Redefine Errors interface #124

torymur opened this issue Dec 12, 2024 · 5 comments
Assignees
Labels
enhancement New feature or request

Comments

@torymur
Copy link
Contributor

torymur commented Dec 12, 2024

We would like to keep the size of Error as small as possible (currently, it's 48 bytes already).

So it needs to be defined as a struct with Boxed inner error, in the same manner as it's done in serde_json or reqwest:
https://github.com/serde-rs/json/blob/master/src/error.rs
https://github.com/seanmonstar/reqwest/blob/master/src/error.rs

@torymur torymur added the enhancement New feature or request label Dec 12, 2024
@sky-2002
Copy link
Contributor

@torymur but with this change, we will have to update code at all places which uses Error::IndexError for example.
Should it then be handled in this way ??

// Convert from ErrorImpl to Error
impl From<ErrorImpl> for Error {
    fn from(err_impl: ErrorImpl) -> Self {
        Error(Box::new(err_impl))
    }
}

And replacing this:

Err(Error::IndexError)

with this:

Err(ErrorImpl::IndexError.into())

Another option is to use constructor methods, I am not sure which design pattern would be better, lmk your thoughts on this.

@torymur torymur self-assigned this Dec 21, 2024
@torymur
Copy link
Contributor Author

torymur commented Dec 21, 2024

Hey @sky-2002

Apologies, I now assigned myself to this issue and I will figure out a better way to communicate issues like this, which is more of a note to not forget, rather than anything actionable (even for a better thought than in description). This part of the project is in the middle of heavy refactoring and is not stabilized yet enough to answer here right now.

@sky-2002
Copy link
Contributor

@torymur ahh right, thanks for clearing, thought as much, it did look like a note to not to forget, my bad 🙌🏻 .
Anyways, apologies for the interruption.
Actually I keep looking for issues to get hands on with rust (as I come from python) and also with outlines, and so found this one.

@torymur
Copy link
Contributor Author

torymur commented Dec 21, 2024

@sky-2002 There are some unassigned issues with json schema, for example, recently outlines added support for objects in enum fields, but we didn't have it here: #100

@sky-2002
Copy link
Contributor

Cool, will check out those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants