mobilityhouse / ocpp

Python implementation of the Open Charge Point Protocol (OCPP).
MIT License
766 stars 298 forks source link

Allow library users to send requests abstracting over specific request type #586

Open reinierl opened 7 months ago

reinierl commented 7 months ago

Sometimes you want to let other code decide which request is to be sent, and just pass stuff on into the OCPP WebSocket connection. In such cases having to different separate methods for the different actions creates a lot of boilerplate, especially with OCPP 2.0.1 which has many more possible actions.

This merge request adds a generic_call method that allows a library user to give a JSON payload and action name, without having to represent the payload as a special object or having to call a specific method for the action.

Note that I'm experienced in OCPP and in coding in general but I'm a noob when it comes to Python. Comments on how to improve my code are very welcome.

OrangeTux commented 7 months ago

Thanks for your PR. And your use case is something we could support.

I would suggest a slightly different implementation, though.

Instead of allowing adding a method that accepts action and payload, I suggest that we create a method that accepts a Call instance. The benefit is that users also control the unique id.

class ChargePoint:
    async def send_call(self, call: Call, suppress=True, unique_id=None):
         ...

Note that I don't like ChargePoint.send_call(), but I'm struggling to find a better name that distinguish it from the existing method ChargePoint.call().

reinierl commented 7 months ago

I had the same naming problem. I like generic_call a bit better than send_call but it still looks clunky.

So if the generic_call method takes a Call object, then the unique ID parameter could be removed couldn't it? So it becomes:

class ChargePoint:
    async def send_call(self, call: Call, suppress=True):
      ...

Would it be OK to still allow callers to pass a Call object with unique_id set to None and let the library generate the unique ID in that case? In my use case I don't need control over what the unique ID is and it's enough to have requests and responses linked through async returns.

I think that's why I went with separate action and payload first. The ugly thing about separate action and payload parameters to me is that the call and return become asymmetric: you call the method with separate parameters of simple types, but it will return you a CallResult object. The ugly thing about having one Call object parameter is that you'd have to set its unique_id field to None to let the library manage the unique_id.

reinierl commented 6 months ago

@OrangeTux I made some changes and I hope it's good to go like this. If you have more ideas about the name and interface for generic_call aka send_call, let me know.

reinierl commented 5 months ago

@OrangeTux @Jared-Newell-Mobility I would like to get this merged. Is there anything I can do to make it happen?

jerome-benoit commented 2 months ago

@OrangeTux @Jared-Newell-Mobility I would like to get this merged. Is there anything I can do to make it happen?

I second the merge of that feature. I need it for OCPP 2 stack testing purpose. What is hindering code owners review and merge?