Skip to content

Make Client methods parameters more intuitive#65

Merged
ion232 merged 4 commits intoion232:mainfrom
EpokTarren:better_default
Sep 10, 2024
Merged

Make Client methods parameters more intuitive#65
ion232 merged 4 commits intoion232:mainfrom
EpokTarren:better_default

Conversation

@EpokTarren
Copy link
Contributor

Improves the types used in Client methods and makes the API of these methods easier by:

  1. Removing parameters which don't have any options.
  2. Introduce Default implementations for Request<_ , _> where it makes sense.
  3. Implements From for some Request<_ , _> and accepts impl Into<Request<_ , _>> in Client methods.

There is a question which came up while writing this PR, model::board::seek::PostRequest has options which couldn't be set before, I left PostRequest::new as is but did implement From<PostQuery>. I think PostRequest::new should accept a PostQuery, though I may be missing something.

Closes #64

For types where Default is obvious it has been implemented,
for types that have one required argument From has been implemented.
Remove arguments which only have a single valid state, instead
initialize these in the function body.
Accept Into<Request<_, _>> to broaden the allowed parameters.
Copy link
Owner

@ion232 ion232 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks for making these changes. Just needs the board::seek fixing and the other suggestion and I'm happy to merge it. It's also got me thinking about how we can improve this further though. I think being able to construct the requests from just mandatory data, or mandatory and optional data would be the next step. Not necessary for this PR though. If you have any thoughts, let me know.

Previously this function did not accept any options despite the internal
state representing the query parameters.
@ion232 ion232 merged commit e4b5cca into ion232:main Sep 10, 2024
@EpokTarren
Copy link
Contributor Author

I think being able to construct the requests from just mandatory data, or mandatory and optional data would be the next step. Not necessary for this PR though. If you have any thoughts, let me know.

Considering types with one non optional parameter I think I got all of the From implementations, as for optional data I think that might be impossible without macros in Rust since there are no variadic functions. Implementing From<(Mandatory, Optional)> would be possible though it feels clunky to me.

FooRequest {
    foo: Foo,
    bar: Option<Bar>
}

impl From<(Foo, Bar)> for FooRequest {
    /* ... */
}

// user
let client = reqwest::ClientBuilder::new().build()?;
let api = LichessApi::new(client, None);
api.foo((foo, bar));

At this point a named type would be easier to read, and if the user has to name that type they might as well name the request type. I think the only real alternative would be just be accepting the number of arguments into the functions or keeping it as is.

@EpokTarren EpokTarren deleted the better_default branch September 10, 2024 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unintuitive Default implementations for requests

2 participants