Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

reinierl
Copy link

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.

@reinierl reinierl requested a review from OrangeTux as a code owner January 30, 2024 21:46
@Jared-Newell-Mobility Jared-Newell-Mobility added the enhancement New feature or request label Feb 2, 2024
@OrangeTux
Copy link
Contributor

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
Copy link
Author

reinierl commented Feb 5, 2024

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
Copy link
Author

@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
Copy link
Author

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

@jerome-benoit
Copy link
Contributor

@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?

@jainmohit2001
Copy link
Collaborator

HI @reinierl, the idea looks promising. Can you please update the PR with master and remove conflicts?
How does raw_call sound to instead of generic_call?

@jainmohit2001 jainmohit2001 requested review from proelke and mdwcrft and removed request for OrangeTux and Jared-Newell-Mobility October 16, 2024 04:33
@reinierl
Copy link
Author

HI @reinierl, the idea looks promising. Can you please update the PR with master and remove conflicts? How does raw_call sound to instead of generic_call?

Done!

@reinierl
Copy link
Author

Hmm, I couldn't quickly find a way to run the tests myself so I just went for it. I'll look into what's failing tomorrow probably.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

5 participants