starred/rspotify

Fork 0

mirror of https://github.com/ramsayleung/rspotify.git synced 2026-04-25 23:45:52 +03:00

[GH-ISSUE #134] Optional parameters #46

New issue

Closed

opened 2026-02-27 20:22:47 +03:00 by kerem · 4 comments

kerem commented

2026-02-27 20:22:47 +03:00

Owner

Originally created by @marioortizmanero on GitHub (Oct 10, 2020).
Original GitHub issue: https://github.com/ramsayleung/rspotify/issues/134

I would like to discuss how to handle optional parameters for this library. I recently made an article inspired by this issue which sums up the different ways to approach this: https://vidify.org/blog/rust-parameters/

I think the most convenient ones in there are the following:

A: Using a bare Option<T>
B: Using Into<Option<T>>
E: Endpoint-oriented interface. We have related groups, like track, album... which could be used as the endpoint implementations.
F: Hybrid derive pattern. More complex but is super nice for the user, and we could find a way to make it easier to implement.
G: There are optional parameters repeated lots of times throughout the code, like market, country, limit... So we might not even need the group implementation, just in the client.

Currently, we use the second one, but it's not consistent. Some functions also use the first one. We should decide one of them and use it consistently throughout the codebase.

My personal favorite is just the first one. Using Into<Option<T>> seems unnecessary to me and doesn't offer that much over a bare Option<T>. That, or approach E, which seems perfect for the parameters I mentioned: market, country... and we could just ignore it when these params are specified for an endpoint that doesn't need it. As long as it's well documented I don't think it should be a problem. market() would just set a value inside the client (similar to the constructor) which would be used for all the calls afterwards.

I'd like to tag @Koxiaet, who opened an issue with a very opinionated list of improvements; I'm sure they can help with this as well :)

Originally created by @marioortizmanero on GitHub (Oct 10, 2020). Original GitHub issue: https://github.com/ramsayleung/rspotify/issues/134 I would like to discuss how to handle optional parameters for this library. I recently made an article inspired by this issue which sums up the different ways to approach this: https://vidify.org/blog/rust-parameters/ I think the most convenient ones in there are the following: **A**: Using a bare `Option<T>` **B**: Using `Into<Option<T>>` **E**: Endpoint-oriented interface. We have related groups, like `track`, `album`... which could be used as the endpoint implementations. **F**: Hybrid derive pattern. More complex but is super nice for the user, and we could find a way to make it easier to implement. **G**: There are optional parameters repeated lots of times throughout the code, like `market`, `country`, `limit`... So we might not even need the group implementation, just in the client. Currently, we use the second one, but it's not consistent. Some functions also use the first one. We should decide one of them and use it consistently throughout the codebase. My personal favorite is just the first one. Using `Into<Option<T>>` seems unnecessary to me and doesn't offer that much over a bare `Option<T>`. That, or approach **E**, which seems perfect for the parameters I mentioned: `market`, `country`... and we could just ignore it when these params are specified for an endpoint that doesn't need it. As long as it's well documented I don't think it should be a problem. `market()` would just set a value inside the client (similar to the constructor) which would be used for all the calls afterwards. I'd like to tag @Koxiaet, who opened an issue with a very opinionated list of improvements; I'm sure they can help with this as well :)

kerem

2026-02-27 20:22:47 +03:00

closed this issue
added the
help wanted

discussion
labels

kerem commented

2026-02-27 20:22:48 +03:00

Author

Owner

@Kestrer commented on GitHub (Oct 17, 2020):

I don't like option E, because if the user wants to call an endpoint that doesn't require the market parameter at best they'll have to choose a random dummy value (which adds unnecessary extra code) and at worst they'll get confused and spend time getting a proper value.

I think that approach F is too complex. Even if we had macros that made it easy to implement, it basically doubles the size of the library, it's not worth it.

Approach G is too limited; it doesn't allow for users to have different markets for different queries. We don't want to force the user to have multiple clients, ever.

Between options A and B I don't have a preference, they both work. I don't think it matters which one is chosen.

PS: what happened to options C and D?

@Kestrer commented on GitHub (Oct 17, 2020): I don't like option E, because if the user wants to call an endpoint that doesn't require the `market` parameter at best they'll have to choose a random dummy value (which adds unnecessary extra code) and at worst they'll get confused and spend time getting a proper value. I think that approach F is too complex. Even if we had macros that made it easy to implement, it basically doubles the size of the library, it's not worth it. Approach G is too limited; it doesn't allow for users to have different markets for different queries. We don't want to force the user to have multiple clients, ever. Between options A and B I don't have a preference, they both work. I don't think it matters which one is chosen. PS: what happened to options C and D?

kerem commented

2026-02-27 20:22:48 +03:00

Author

Owner

@marioortizmanero commented on GitHub (Oct 17, 2020):

The thing with E is that if the endpoint doesn't need the parameter it doesn't have to be specified. Its value will only be used for endpoints that require it. But yeah, I'm skeptical about it as well.

C and D seemed too basic to me and not exactly what we want. Of course, they can also be discussed. The main issue is that we'd need to declare both a function and a struct, and the user would have to import both of them as well, and with so many endpoints and so little of them with the exact same parameters I'm not sure if it's the most appropiate choice.

@marioortizmanero commented on GitHub (Oct 17, 2020): The thing with E is that if the endpoint doesn't need the parameter it doesn't have to be specified. Its value will only be used for endpoints that require it. But yeah, I'm skeptical about it as well. C and D seemed too basic to me and not exactly what we want. Of course, they can also be discussed. The main issue is that we'd need to declare both a function and a struct, and the user would have to import both of them as well, and with so many endpoints and so little of them with the exact same parameters I'm not sure if it's the most appropiate choice.

kerem commented

2026-02-27 20:22:48 +03:00

Author

Owner

@Kestrer commented on GitHub (Oct 17, 2020):

Ooh, the letters came from your blog post. I didn't realize that and got really confused why two letters were skipped :P.

@Kestrer commented on GitHub (Oct 17, 2020): Ooh, the letters came from your blog post. I didn't realize that and got really confused why two letters were skipped :P.

kerem commented

2026-02-27 20:22:48 +03:00

Author

Owner

@marioortizmanero commented on GitHub (Apr 19, 2021):

After working a bit on #201 and working with function signatures, I've come to the conclusion that using so many unnecessary generic parameters with Into<Option<T>> is a mess. If anything I'd go for the simplest; option A.

Once #201 is done we'll get rid of the majority of optional parameters anyway, since when using iterators limit or offset don't need to be passed, so this should be less of a problem. The wonderful part about iterators is that the user can just use .take() or .take_while(), which won't cause unnecessary requests and can work as a limit.

@marioortizmanero commented on GitHub (Apr 19, 2021): After working a bit on #201 and working with function signatures, I've come to the conclusion that using so many unnecessary generic parameters with `Into<Option<T>>` is a mess. If anything I'd go for the simplest; option A. Once #201 is done we'll get rid of the majority of optional parameters anyway, since when using iterators `limit` or `offset` don't need to be passed, so this should be less of a problem. The wonderful part about iterators is that the user can just use `.take()` or `.take_while()`, which won't cause unnecessary requests and can work as a `limit`.

kerem referenced this issue

2026-02-27 20:23:46 +03:00

[PR #46] [MERGED] Add a missing reference to current_user_recently_played #207