Add configurable timeout to API config #24
Conversation
src/config/api.rs
Outdated
| @@ -64,6 +64,8 @@ pub struct ApiConfig { | |||
| pub default_model: Option<String>, | |||
| #[serde(skip_serializing_if = "Option::is_none")] | |||
| pub version: Option<String>, | |||
| #[serde(skip_serializing_if = "Option::is_none")] | |||
| pub timeout: Option<u16>, | |||
There was a problem hiding this comment.
quibble:
| pub timeout: Option<u16>, | |
| /// client timeout in seconds | |
| pub timeout: Option<u16>, |
Was unsure about renaming that to timeout_seconds or adding a comment.
There was a problem hiding this comment.
Good point, I'd vote for timeout_seconds myself since then the meaning is clear and unambiguous on the config file itself
src/config/api.rs
Outdated
| @@ -94,13 +96,23 @@ impl ApiConfig { | |||
| .unwrap_or_default() | |||
| } | |||
|
|
|||
| pub fn get_timeout(&self) -> Option<Duration> { | |||
There was a problem hiding this comment.
Small comment on the default behavior: I'd prefer to have the default method of the API config set it to 30s and use None for actual no timeout instead of zero.
I find less confusing to rely on the type system when possible instead of defining arbitrary behavior 🙂
There was a problem hiding this comment.
Right, that makes sense. I'll try and get the changes in within the next day or so
|
Hey, no worries! Always happy to chip in :) Anyway, I'd love to give the changes a go on my end, if only to practice my real world Rust a little more. If it ends up not to your taste afterwards, I wouldn't mind at all if you took over and added on top of my modifications. Does that work? |
Change the default timeout to 30 and treat its omission as indefinite timeout Rename timeout to timeout_seconds
|
Alright, I've implemented the requested changes. These should cover what we've discussed, unless I misunderstood something. |
|
Hey, no worries! Lmk if you'd prefer I squashed the commits together or anything like that and I'll get to that ASAP. |
|
It's perfect like that! |
I'm currently experimenting with ollama on a not too powerful laptop, so I stumbled upon #22 pretty quickly.
Seeing as I liked the simplicity of smartcat and the fact it's Rust based, I figured I'd try my hand at adding an optional timeout parameter.
I implemented the "timeout" field in ApiConfig, and made it optional. I also made it so the reqwest client is instantiated through its builder so that the timeout can be specified. It could also be passed on the individual request, but since that form doesn't take an Option as a parameter, I figured this would be a bit cleaner.
Right now, the logic works as such: