Proposal: Remove positional-or-keyword parameters from the ReqClient

[onyx-and-iris]

Hi, I'd like to propose a major change to the codebase, removal of positional-or-keyword arguments in the ReqClient methods in favour of keyword-only arguments.

Motivation

There are two main motivations for this proposal:

Currently the ReqClient is inflexible to change.

Take this simple example:

    def set_scene_name(self, old_name, new_name):

At the time this method was written it only needed to accept two parameters, the current scene name and the new scene name. So that's fine, however, since then uuid's have been introduced to the protocol and many of the methods now accept either a name or a uuid. Unfortunately, as in this example, we cannot reasonably add scene_uuid because both it and old_name would need to be optional. Making old_name optional would require repositioning it therefore breaking the method.

This leads onto the second motivation.

Bring ReqClient method parameter names inline with the protocol.

Looking through the ReqClient methods it's clear that some of the function parameters were named with usability in mind, for example:

    def set_scene_scene_transition_override(self, scene_name, tr_name, tr_duration):

That's great, however, because the parameter names don't match the protocol field names it means we are unable to test the ReqClient methods against the protocol.json programmatically. Doing so would benefit both maintainers and users of the package because it would speed up the process of adding new methods, ensure parameters are properly marked as optional/required, ensure there are no missing methods and generally make life easier.

Currently this option isn't open to us since renaming the parameters would be a breaking change.

What this might look like

As an example of how this would affect users of the package:

A method like this:

    def set_source_filter_enabled(self, source_name, filter_name, enabled):

would become:

def set_source_filter_enabled(
    self,
    *,
    source_name: Optional[str] = None,
    source_uuid: Optional[str] = None,
    filter_name: str,
    filter_enabled: bool,
):

and instead of calling it like this:

# positional
client.set_source_filter_enabled("Scene", "Chroma Key", True)
# or keyword
client.set_source_filter_enabled(source_name="Scene", filter_name="Chroma Key", enabled=False)

users would need to call it like this:

# keyword-only with argument names that match the protocol field names but snake cased
client.set_source_filter_enabled(
    source_name="Scene",
    filter_name="Chroma Key",
    filter_enabled=True,
)

Consequences

Upsides

• We can deliver a more consistent and error free library and hopefully keep on top of new additions to the protocol at a faster rate.
• Consistency between parameter names and protocol fields should reduce some cognitive load when using named parameters.
• The ReqClient becomes more flexible and we free ourselves from the constraints of keyword-or-positional arguments. This is especially important because we don't control the underlying protocol.

Downsides

• This would introduce a breaking change and require a major version bump. Consequently, current and past users of the package will need to update their code if they want to keep using obsws-python with future updates.
• Certain otherwise short function calls will become more verbose, for example:

# before
client.set_current_program_scene("Scene 1")

# after
client.set_current_program_scene(scene_name="Scene 1")

---

obsws-python is three years in age now and as package authors we've been careful not to introduce reckless breaking changes. This wouldn't be reckless and on the balance of things I think the benefits justify the drawbacks.

Let me know your thoughts.

[aatikturk]

Hi @onyx-and-iris

Let me thank you first for all the efforts for maintaining this package and of course alway introducing valuable inputs.

• I agree with you on the keyword parameters issue. however I don't know how we should implement this change while minimizing bad experiences that our users might have.

• For the parameter names issue, yes they were named with usability in mind at the time. but now maintaining the package is getting harder as the protocol introduces any changes.

I think this would be best handled as a 2-phase change.

1. introduce keyword parameters (naming would be inline with protocol param names) along with existing positional parameters. also adding a warning about positional parameter deprecation in ReqClientclass may help for current users of the package

2. after a set period of time removing all positionals and using only keyword parameters.

This approach may seem daunting as we go through the ReqClient twice but we'll solve both problems.

Another option would be converting current parameters to keyword-only parameters, keeping the names as they're. and adding new parameters where needed.

I can only think of two options right now. But I'm open to discuss any suggestions you might have.

thanks
Adem

[onyx-and-iris]

Thanks for the quick reply. I think if the goal is to minimise the impact on the users of the package then splitting the update into two breaking changes instead of one would have the opposite effect. However, that shouldn't be the only goal, in fact, we also have to think about the impact on us as maintainers.

Regarding the first option, if we were to rename the keyword arguments and mark them as keyword-only and then later, remove the positional-or-keyword parameters and replace them with keyword-only parameters, that would be one breaking change followed by another.

The second option only solves one of the problems because we'd be converting the parameters to keyword-only while not bringing the names inline with the protocol.

In my opinion neither of those routes makes sense. They increase the workload on us as maintainers, the path forward would be less clear, the first option introduces two breaking changes instead of one and the second option only solves one of the problems.

If we're going to make this change then we should solve all problems at once. Look, if the users have to rewrite their function calls to make parameters keyword-only then why not just rename them at the same time? A lot of people will have called the functions without naming the parameters anyway, since they're currently positional-or-keyword. I know I have in much of the code I've written. Splitting the process in two, increasing the burden on us as well as the users doesn't make sense to me.

I should point out that the work has already been done, you can check it out on this branch. I wouldn't have made the proposal without first thoroughly investigating the idea.

[aatikturk]

Hi @onyx-and-iris,

I've already gone through the points you mentioned about our options before and after your message.

Splitting the process in two, increasing the burden on us as well as the users doesn't make sense to me.

That's what I was thinking when I say option-1 may seem daunting. Because we'll have to refactor twice to solve both problems.

I'm still not sure about making two breaking changes at once. However, since you've already put in required work into your branch (as always) and also for the sake of future benefits both for us maintainers and for users, let's cross our fingers and call it obsws-python v2.0!

Once you create the PR, if anything else should be discussed during review feel free to ping me (sorry we still can't use discord here)

[onyx-and-iris]

Alright, thanks for your input. I'm going to have to give this more time because there's quite a bit more that needs doing, I can't just make a PR of that branch. Tests, examples and documentation need to be updated. Tbh this project should really have a CHANGELOG, it helps a great deal in situations like this.

Btw, there is another option we have that hasn't been discussed. We can just scrap the proposal, it is afterall, a proposal. We aren't being flooded with issues asking us to add things that can't be added, and uuid's are less important for websocket code than say, when using the integrated obspython library. However, that doesn't avoid all the things detailed in the proposal, the underlying issue with the ReqClient would still exist. My prediction is that sooner or later, something will get added/changed in the underlying protocol that we won't be able to update due to the reliance on positional-or-keyword parameters.

I will close this for now and reopen when I can give it more time. In fact, it would be useful if you could open up the discussions section for obsws-python. This along with some of the other posts would better fit there imo.