ocpp
ocpp copied to clipboard
Add dataclasses for ChargingProfile and its constituents
Some questions:
-
These objects don't quite fit into calls.py, because they're not call payloads. On the other hand, they don't really fit in enums.py either. I considered adding a new file like
types.py, but figured it best to see what your preference is? -
I also wanted to add a test to ensure a SetChargingProfilePayload is serialised to the correct representation when using these objects. I noticed the codebase doesn't have such tests for other payloads, so I left it. Would you like to keep it this way?
Thanks!
No problem!
Good point regarding the de-serialisation, I had missed that. I'll look to make changes to support this.
Apologies, I don't quite understand the backwards incompatibility. My understanding when the payload is passed to Call all dataclasses are converted to dictionaries (similarly before any validation), and there is no enforcing of the types on the dataclass, and thus any code which is passing dictionaries to cs_charging_profiles would continue to work. Similarly, my understanding is we don't de-serialise back to the payload class automatically anywhere (nor validate against the dataclass). I do see a backwards incompatibility in static type checking (i.e. mypy), and if this is your concern I would propose we modify the type to still accept dicts (cs_charging_profiles: Untion[ChargingProfile, Dict]) ? Or am I missing something?
Changing type of SetChargingProfilePayload.cs_charging_profiles from Dict to ChargingProfile is a backwards incompatible change.
However, you're right when you write "my understanding is we don't de-serialise back to the payload class automatically anywhere ". In practice it means users implementing a @on() handler for SetChargingProfile still get cs_charging_profiles as a Dict.
Changing the type hints, as you suggested, is required to express that cs_charging_profiles can be both Dict or ChargingProfile. And with this modification this PR is backwards compatible again.
I think for v1.0.0 we should drop Dict as possible type for cs_charging_profiles and only allow ChargingProfile. That means this library should automatically deserialize incoming SetChargingProfile requests into ChargingProfile and the other nested dataclasses. But these change are in the scope of a different issue.
Ok, I follow you now. I'll go with Union for now then to avoid making a backwards incompatible change.
I agree with the changes on (de)serialisation for v1.0.0 - we're doing this internally on top of the library at the minute at it would be handy to have it upstream!
remove_nones needed to change to remove keys where the value is None for nested dicts, and dicts which contains lists of dicts. These structures appear in the charging profile code. I've added that in this commit https://github.com/mobilityhouse/ocpp/pull/172/commits/19bfe11a87e35cfa296a9d2ee68c33e834de975c
If we agree that deserialisation of dicts to dataclasses is out of scope and something for v1.0 then I think this PR is ready to go.
There is a PR I'd like to submit and was working on a variation of your change to remove_nones. +1 on that change from me.
@OrangeTux @tropxy I think this is ready to go. Are there any other changes you need?