twitch-rs / twitch_api

Rust library for talking with the Twitch API aka. "Helix", TMI and more! Use Twitch endpoints fearlessly!
Apache License 2.0
151 stars 33 forks source link

Builder pattern + methods #47

Open Emilgardis opened 3 years ago

Emilgardis commented 3 years ago

Currently, this crate uses the builder pattern to assure a stable API i.e https://github.com/Emilgardis/twitch_api2/blob/e1ba9faff6c72b92f271a888b480a016c334b82a/src/helix/channels.rs#L82-L88

I decided to do it this way due to Twitch not guaranteeing a stable API themselves on documented items, so marking structs as #[non_exhaustive] and using a builder seems better.

However, sometimes this makes things harder to use, as typed-builder does not exactly make structs more accessible.

While this is done for stability, I feel like some structs don't lend themselves to the builder pattern, like the above example.

I want to solve this somehow, without sacrificing API stability.

To do that I think what needs to be done is creating methods for common usages of requests.

Example:

impl GetChannelInformationRequest {
    pub fn get_channel(broadcaster: String) -> GetChannelInformationRequest {
        GetChannelInformationRequest::builder().broadcaster_id(broadcaster).build()
    }
}

or for https://github.com/Emilgardis/twitch_api2/blob/e1ba9faff6c72b92f271a888b480a016c334b82a/src/helix/games.rs#L59-L68

impl GetGamesRequest  {
    pub fn get_game(id: types::CategoryId) -> GetGamesRequest {
        GetGamesRequest::builder().id(vec![id]).build()
    }
}

There is the crate derive_builder that could help also.

ModProg commented 3 years ago

I want to add to this that it makes it really difficult to optionally set some fields.

This is the simplest I could do this, but Maybe there is also a better way, I'm not seeing:

pub struct ChannelInformation<'a> {
    title: Option<&'a str>,
    language: Option<&'a str>,
    game: Option<&'a str>,
}
impl ChannelInformation<'_> {
    fn to_modify_body(&self) -> ModifyChannelInformationBody {
        match (self.title, self.game) {
            (Some(t), Some(g)) => ModifyChannelInformationBody::builder()
                .title(t)
                .game_id(g)
                .broadcaster_language(self.language.map(ToOwned::to_owned))
                .build(),
            (Some(t), None) => ModifyChannelInformationBody::builder()
                .title(t)
                .broadcaster_language(self.language.map(ToOwned::to_owned))
                .build(),
            (None, Some(g)) => ModifyChannelInformationBody::builder()
                .game_id(g)
                .broadcaster_language(self.language.map(ToOwned::to_owned))
                .build(),
            (None, None) => ModifyChannelInformationBody::builder()
                .broadcaster_language(self.language.map(ToOwned::to_owned))
                .build(),
        }
    }
}
Emilgardis commented 3 years ago

this is due to strip_option, should consider not using it at all and just rely on into

spikespaz commented 7 months ago

How does typed_builder currently work? The builder type is private, the build function is crossed-out and RA fills in a single parameter channel_points_custom_reward_redemption_add_v1_builder_error_missing_required_field_broadcaster_user_id. I can't figure out how to finalize the builder.

Emilgardis commented 7 months ago

@spikespaz the typed_builder crate creates a builder that throws a compile error if setters are missing, the parameter you get contains the missing setter, you need to add broadcaster_user_id. Anyway, I'd recommend using ChannelPointsCustomRewardRedemptionAddV1::broadcaster_user_id(...) instead which is what this issue is about. Looks like I forgot to link the pr that created them: #262