Improvement on endpoints API

[rafaellehmkuhl]

The context:

We had a class called Endpoint, that receives 3 arguments: connection_type, place and argument. Right now, with the new metadata, this class receives 4 more arguments: name, owner, persistent and protected, with the last two having default false values.

The problem:

Two many arguments to instantiate an endpoint, with some of those arguments being unrelated (some are metadata and some are connection data).

The solution:

Separate the Endpoint class into 3 classes:

• ConnectionEndpoint
• MavlinkEndpoint
• CompanionEndpoint

The base ConnectionEndpoint class would be abstract, and represents what an Endpoint should do, but not how.

MavlinkEndpoint would inherits from ConnectionEndpoint and implement it (would have basically all the code that is in Endpoint today).

CompanionEndpoint would be a class that has the companion metadata we want to add (name, owner, persistent and protected) and receives also an ConnectionEndpoint instance.

In that way, we better separate the concerns (ConnectionEndpoint instance will store connection information while CompanionEndpoint instance stores the metadata), we make the calls smaller and more clear and we open the possibility for future implementations of new protocols.

[patrickelectric]

I would recommend to make MavlinkEndpoint a builder, where you can configure it directly with:

MavlinkEndpoint.create()
    .with_endpoint(EndpointData(Endpoint.TCPout, "0.0.0.0", 5600))
    .with_configuration(EndpointMetadata(name, owner, persistent, protected))
# and
MavlinkEndpoint.create()
    .with_endpoint(EndpointData(Endpoint.TCPout, "0.0.0.0", 5600))
    .new_configuration()
        .set_name(name)
        .set_protected()

As you can see this makes it simpler and much readable.

• I'm not sure why MavlinkEndpoint should inherits ConnectionEndpoint, who else will ? If there is no reason to have such abstracted class you should make the implementation inside MavlinkEndpoint and use the others classes as simple structures to abstract the data.
• CompanionEndpoint is not a good name, since is not really an endpoint, this may be an EndpointMetadata, as you can see in my example, since the name matches exactly your description.

[rafaellehmkuhl]

• About the ConnectionEndpoint, I was thinking about opening the doors for future implementations of different protocols, which demand different connection arguments, but it will have so many other implications and is definitely too much for now. Let's focus on separating the connection-data/metadata.
• I'm thinking that the CompanionEndpoint is an endpoint since it has connection information (through the MavlinkEndpoint argument it receives), but the names you propose are also good.
• About the builder, I liked the syntax, but don't know if really fits in our use case. Builders are usually used for flexible constructions, when you have mandatory and non-mandatory data. Both metadata and connection-data are mandatory, and a builder turns things non-mandatory (I should never create an endpoint that does not have connection-data or metadata). Mixing my original idea with yours would get to something like this::

CompanionEndpoint(
    EndpointMetadata(name, owner, persistent, protected),
    MavlinkEndpoint(connection_type, place, argument)
)

[patrickelectric]

• The problem with that approach is that if metadata needs extra fields it'll start the problem all over again.
• Not all fields are necessary, you can have protected and persistent off my default and only true require name and owner.

[rafaellehmkuhl]

• The problem with that approach is that if metadata needs extra fields it'll start the problem all over again.
• Not all fields are necessary, you can have protected and persistent off my default and only true require name and owner.

• If metadata happens to need extra required arguments, there's really nothing we can do besides adding them, right?
• Yes. persistent and protected are already defaulted to false on #166

[patrickelectric]

If metadata happens to need extra required arguments, there's really nothing we can do besides adding them, right?

Yes, you can use a builder validation.

MavlinkEndpoint.builder()
    .with_endpoint(EndpointData(Endpoint.TCPout, "0.0.0.0", 5600))
    .new_configuration()
        .set_name(name)
        .set_protected()
        # missing required
    .create() # create will check the validation and trow or return an optional/result

[rafaellehmkuhl]

What would be the advantage of doing that instead of the below?

CompanionEndpoint(
    EndpointMetadata(
        name=name, 
        owner=owner, 
        new_required=new_required,
        persistent=persistent,
        protected=protected,
        new_optional=new_optional
    ),
    MavlinkEndpoint(
        connection_type=connection_type,
        place=place,
        argument=argument
    )
)