Change class constructors to **kwargs

[tayler6000]

Our current constructor for VoIPPhone has a ton of different parameters:

def __init__(
    self,
    server: str,
    port: int,
    user: str,
    credentials_manager: CredentialsManager,
    bind_ip="0.0.0.0",
    bind_port=5060,
    transport_mode=TransportMode.UDP,
    cert_file: Optional[str] = None,
    key_file: Optional[str] = None,
    key_password: KEY_PASSWORD = None,
    call_callback: Optional[Callable[["VoIPCall"], None]] = None,
    rtp_port_low=10000,
    rtp_port_high=20000,
    callClass: Type[VoIPCall] = None,
    sipClass: Type[SIP.SIPClient] = None,
):
    ...

VoIP and SIP are extremely dynamic, and we often get feature requests to cover a new use case. The parameters for these use cases will continue to grow. I recommend we switch the constructor to

def __init__(self, **kwargs):
    ...

I feel this will increase readability a lot, however, it will kind of break LSP. Also, I like the idea of creating a helper function that can take a dictionary and change it to be a kwargs compatible dictionary. This way, sys admins can create a single config file for multiple pyVoIP instances to enable horizontal scaling among other things.

Thoughts?

[tayler6000]

@Input-BDF @xyc0815 Thoughts?

[tayler6000]

One thing is that LSPs wouldn't know what kwargs are in use so we'd have to make sure classes had docstrings.

[jrozhon]

Hi, sorry for being a bit late to the party. From my perspective (mediocre programmer at best), this would only make things less clear and more difficult for static type checkers. I would definitely prefer grouping the related arguments into a single model (ie, pydantic or dataclass), such as:

class Remote(BaseModel):
    server: str
    port: int
...

class Tls(BaseModel):
    cert_file: Path
    key_file: Path
...

and then using these as arguments to the initialization method. I think it also follows something called dependency injection or something like that from the SOLID principles, but I never remember these correctly.

By using this approach, you would simplify the initializer and introduce a strict type checking (pydantic) and possibly allow for usage of parse_obj or even parse_raw to be used to create instances from jsons, or whatever. Performance hit can be neglected, I believe, as you are creating an instance just once.

The downside is that to make the VoIPPhone instance you would need to create several other objects.

J

[jrozhon]

One more thought, pydantic also works great with environment variables, so storing the passphrase to a private key would also be closer to the common patterns.

[tayler6000]

@jrozhon

I didn't think about type checking, so I agree with everything you said about using data classes. This is basically what I've done with CredentialsManager in 2.0. I've worked hard to avoid dependencies in this project, though I wouldn't be fully against dependencies in future. That being said, I don't think Pydantic would be a good fit for pyVoIP.

[jrozhon]

Sure, I tend to overuse pydantic as it is addictive :-). dataclasses are much lighter and would do just fine, but the idea stands.

[xyc0815]

I wouöd like to change the function calls, with a big amount of elements, to **kwags. When we also want to add features like proxy support we need even more elements and not everyone needs all this elements. Also I would look where call elements are not needed or included in another call element like in the class VoIPCall. The VoIPCall gets the class VoIPPhone als call element and the call elements bind_ip and sendmode, both are also inside VoIPPhone.

class VoIPCall:
    def __init__(
        self,
        phone: "VoIPPhone",
        callstate: CallState,
        request: SIP.SIPMessage,
        session_id: int,
        bind_ip: str,
        ms: Optional[Dict[int, RTP.PayloadType]] = None,
        sendmode="sendonly",
    ):

[xyc0815]

After a few considerations, I believe that a dataclass makes more sense to pass the data. With **kwags you can simply pass any amount of data, but if you have mandatory data, you have to pass it additionally in the class call. In a dataclass you can do both at once. In a dataclass yo also can keep the datatypes of the elements.
I've done some examples first for the dataclass. This must be called like:
VoIPPhoneParameter(sever='test.test.com', user='admin', port=4196)
Everthing else is optinal.

@dataclass
class VoIPPhoneParameter:
    server: str
    port: int
    user: str
    credentials_manager: Optional[CredentialsManager]
    bind_ip: Optional[str] = "0.0.0.0"
    bind_port: Optional[int] = 5060
    transport_mode: Optional[TransportMode] = TransportMode.UDP
    cert_file: Optional[str] = None
    key_file: Optional[str] = None
    key_password: Optional[KEY_PASSWORD] = None
    callback: Optional[Callable[["VoIPCall"], None]] = None
    rtp_port_low: Optional[int] = 10000
    rtp_port_high: Optional[int] = 20000
    callClass: Type[VoIPCall] = None
    sipClass: Type[SIP.SIPClient] = None


class VoIPPhone:
    def __init__(
        self,
        voip_phone_parameter: VoIPPhoneParameter
    ):
        # data may transferred in voi
        self.voip_phone_parameter = voip_phone_parameter
        if self.voip_phone_parameter.rtp_port_low > self.voip_phone_parameter.rtp_port_high:
            raise InvalidRangeError(
                "'rtp_port_high' must be >= 'rtp_port_low'"
            )
        self.callClass = self.voip_phone_parameter.callClass is not None and self.voip_phone_parameter.callClass or VoIPCall
        self.sipClass = self.voip_phone_parameter.sipClass is not None and self.voip_phone_parameter.sipClass or SIP.SIPClient
        # data defined in class
        self._status = PhoneStatus.INACTIVE
        self.NSD = False

This call with **kwags can be called empty like VoIPPhone().

class VoIPPhone:
    def __init__(
        self,
        **kwags
    ):
        # data may transferred in ""kwags
        self.rtp_port_low = kwags.get('rtp_port_low', 10000)
        self.rtp_port_high = kwags.get('rtp_port_high', 20000)
        if self.rtp_port_low > self.rtp_port_high:
            raise InvalidRangeError(
                "'rtp_port_high' must be >= 'rtp_port_low'"
            )

        self.server = kwags.get('server')
        self.port = kwags.get('port')
        self.bind_ip = kwags.get('bind_ip')
        self.user = kwags.get('user')
        self.credentials_manager = kwags.get('credentials_manager')
        self.callback = kwags.get('call_callback')
        self.transport_mode = kwags.get('transport_mode')
        self.cert_file = kwags.get('cert_file', None)
        self.key_file = kwags.get('key_file', None)
        self.key_password = kwags.get('key_password', None)

        self.callClass = kwags.get('callClass', None) is not None and kwags.get('callClass', None) or VoIPCall
        self.sipClass = kwags.get('sipClass', None) is not None and kwags.get('sipClass', None) or SIP.SIPClient
        # data defined in class
        self._status = PhoneStatus.INACTIVE
        self.NSD = False