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

Add call unique id to handler kwargs #545

Merged
merged 9 commits into from
Jan 17, 2024
Merged

Conversation

wafa-yah
Copy link
Contributor

@wafa-yah wafa-yah commented Dec 14, 2023

At the level of ChargePoint.handle_call, currently the library only passes the message payload to the _on_action, and _after_action handlers.
For some features like logging the messages being treated by those handlers, it would be nice to pass the Call.unique_id as well.

⚠️ This will be a breaking change though as all the handlers defined by the library clients should always define call_unique_id in the kwargs, hence the label For inclusion in release 1.0.0 => Not a breaking change anymore after introducing the signature check.

Issue Link: #544

@OrangeTux
Copy link
Contributor

I share your need for allowing users to access the request id. Currently, this library is lacking at that point.

But I'm hesitant about this PR. It is a breaking change to the main API: the handler. So potentially, it breaks a lot of code for users of this library.

I'm thinking out loud here and considering some alternative implementations:

  1. Would it be possible to inspect the interface of a handler and only pass the unique id to the handlers when an argument unique_id is part of the signature?
  2. Alternatively, in Flask, users have access to the request context. Maybe we can do something similar using ContextVars..

I'm going to be unavailable during the Holidays, so I might respond in a few weeks.

@wafa-yah
Copy link
Contributor Author

wafa-yah commented Jan 4, 2024

I share your need for allowing users to access the request id. Currently, this library is lacking at that point.

But I'm hesitant about this PR. It is a breaking change to the main API: the handler. So potentially, it breaks a lot of code for users of this library.

I'm thinking out loud here and considering some alternative implementations:

1. Would it be possible to inspect the interface of a handler and only pass the unique id to the handlers when an argument `unique_id` is part of the signature?

2. Alternatively, in Flask, users have access to [the request context](https://flask.palletsprojects.com/en/3.0.x/reqcontext/). Maybe we can do something similar using[ ContextVars.](https://docs.python.org/3/library/contextvars.html).

I'm going to be unavailable during the Holidays, so I might respond in a few weeks.

@OrangeTux makes sense.
I applied the suggestion of checking the signature, should not be a breaking change now.

@wafa-yah wafa-yah requested review from OrangeTux and removed request for OrangeTux January 4, 2024 19:44
@Jared-Newell-Mobility Jared-Newell-Mobility changed the title ISSUE-544 add call unique id to handler kwargs Add call unique id to handler kwargs Jan 11, 2024
Copy link
Contributor

@Jared-Newell-Mobility Jared-Newell-Mobility left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clear definition in the tests

@Jared-Newell-Mobility Jared-Newell-Mobility merged commit f32a86d into master Jan 17, 2024
5 checks passed
@Jared-Newell-Mobility Jared-Newell-Mobility deleted the ISSUE-544 branch January 17, 2024 12:21
ajmirsky pushed a commit to ajmirsky/ocpp that referenced this pull request Nov 26, 2024
Issue Link: mobilityhouse#544

The `call_unique_id` is passed to the `on` and `after` handlers only if it is explicitly set in the handler signature.
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.

3 participants