Make API values optional

[muesli]

It would be nice if values like SetSceneItemPropertiesParams's Locked or Rotation were pointers to their values, making setting them optional. Currently forgetting to set that member value will essentially reset the Locked status or Rotation.

The biggest risk I see here is that newer versions of the OBS Websockets API and this library will introduce new member values. Bumping the goobs dependency (e.g. in obs-cli) is a bit risky then, as any unhandled member values could potentially cause unexpected behavior.

See: muesli/obs-cli#34

[andreykaipov]

I expected something like this to happen when I added omitempty to every field (see #12 (comment)). I had to leave them off boolean fields because omitting a zero-value of false would make something like unlocking a source impossible. Pointers to the bools would indeed solve this problem since it'd give the bool 3 states (yes, no, and not present), but I think the UX would be so ugly.

params := &sceneitems.SetSceneItemPropertiesParams{
	SceneName: "main",
	Item:      &typedefs.Item{Name: "bg"},
	Visible:   &[]bool{false}[0],
	Locked:    &[]bool{false}[0],
}

I suppose there's no getting around this tradeoff if it makes the library more robust and consistent though, is there?

[andreykaipov]

Hmm... so after making the change in that PR, I realized it also changes response bodies returning boolean values. Do you think this is okay? I could have only request bodies expect pointers to bools, but that inconsistency would bother me.

[muesli]

Hmm... so after making the change in that PR, I realized it also changes response bodies returning boolean values. Do you think this is okay? I could have only request bodies expect pointers to bools, but that inconsistency would bother me.

It would probably feel more natural to have values in the response body, as their value isn't optional in that case - unless you plan to support multiple versions of the websockets API and want nil to indicate that a certain value hasn't been returned by the OBS instance.

[muesli]

I suppose there's no getting around this tradeoff if it makes the library more robust and consistent though, is there?

I'm afraid I agree. It's not exactly nice, but it's the correct solution to this tri-state (true, false, unknown).

There are two alternatives I can think of to make the code a bit more readable:

1. Internal helper method for convenience:

func BoolPtr(b bool) *bool {
    boolVar := b
    return &boolVar
}

2. Functional options, which can actually turn out to be really readable:

scenes.NewSetCurrentSceneRequest().
    WithSceneName(name).
    WithFoo(bar).
    With...

A third option would involve reflection, but I think we'd want to avoid that 😆

[andreykaipov]

Sorry for the wait!

It would probably feel more natural to have values in the response body, as their value isn't optional in that case.

This makes sense. Thank you for the feedback 🙏 I added a conditional to set *bools on the request bodies only (4d508be).

2. Functional options, which can actually turn out to be really readable:

scenes.NewSetCurrentSceneRequest().
    WithSceneName(name).
    WithFoo(bar).
    With...

I do really like this. It's similar to the funcops I have for the constructor but chained. I'd have to explore this pattern when I take a stab at the v5 protocol. Would be cool to offer both APIs.

[andreykaipov]

Released in v0.8.0!