ion232 / lichess-api

A Rust API client for lichess.org
https://lichess.org/api
Apache License 2.0
9 stars 7 forks source link

Unintuitive Default implementations for requests #64

Closed EpokTarren closed 1 month ago

EpokTarren commented 1 month ago

Currently the following code compiles, it does not have what I would imagine is the expected behaviour, instead the final line will always error, this part of the crate API feels like a footgun.

let client = reqwest::ClientBuilder::new().build()?;
let api = lichess_api::LichessApi::new(client, None);

api.get_profile(Default::default())?;

My suggestion is that model::Request shouldn't implement Default directly, instead for types such as model::account::profile::GetRequest should implement so that the request is properly formed, mainly the path is currently missing. For request types such as model::messaging::inbox::PostRequest it would make sense not to implement Default at all. Do methods which take no arguments need a to accept a request type or could it be generated in the method body?

For methods such as client::LichessApi::tv_channel_games that only have one required parameter, could they accept only that in addition to the full type using From / Into, example code below.

impl From<ChannelName> for GetRequest {
    fn from(channel: ChannelName) -> GetRequest {
        GetRequest::new(channel, None)
    }
}
impl LichessApi<reqwest::Client> {
    pub async fn tv_channels(&self, request: impl Into<channels::GetRequest>) -> Result<Channels> {
        self.get_single_model(request.into()).await
    }
}

Would these changes be desirable contributions to the project?

ion232 commented 1 month ago

Thanks for your feedback :)