search

pepperoni21 / ollama-rs

A Rust library allowing to interact with the Ollama API.
MIT License
367 stars 47 forks source link

Improve Errors: Return Result<T,E> instead of Result<T,String> #15

Closed SimonCW closed 5 months ago

SimonCW commented 5 months ago

Hi @pepperoni21,

thanks for the library! I've been using it for a bit and stumbled over functions returning Result<T, String> instead of <T, Error> (e.g. the generate() function. IMHO, this decreases the ergonomics, e.g. makes it harder to surface errors, harder to use anyhow::context(), ...

Would you be open for a PR that introduces error types for the client? I'm relatively new to Rust but this is something I can pull off with a bit of guidance. I'd keep to the recommendations shared here: https://youtu.be/jpVzSse7oJ4?si=N4zw1Wg7fVsQ8Mdl

This would probably introduce the thiserror crate as a dependency, increase compile time a little bit, and constitute a breaking change (I think).

Let me know what you think Simon

pepperoni21 commented 5 months ago

Hi @pepperoni21,

thanks for the library! I've been using it for a bit and stumbled over functions returning Result<T, String> instead of <T, Error> (e.g. the generate() function. IMHO, this decreases the ergonomics, e.g. makes it harder to surface errors, harder to use anyhow::context(), ...

Would you be open for a PR that introduces error types for the client? I'm relatively new to Rust but this is something I can pull off with a bit of guidance. I'd keep to the recommendations shared here: https://youtu.be/jpVzSse7oJ4?si=N4zw1Wg7fVsQ8Mdl

This would probably introduce the thiserror crate as a dependency, increase compile time a little bit, and constitute a breaking change (I think).

Let me know what you think Simon

Hey, thanks for your suggestion. Actually I did make an error type in https://github.com/pepperoni21/ollama-rs/blob/master/src/error.rs, I guess I just forgot to use it in that method.

But if you think you can make some improvements feel free to make a PR, because I know error handling could be better.