v201:call,call_result,datatypes: complete and corrected type annotations

[malsyned]

I groveled through the v2.0.1 spec and updated all of the type signatures to be complete and accurate to the best of my ability.

I did this to support some work I'm doing to make a variant of @on that converts the JSON back to Python objects, but I think it will be useful to anyone using the library with an IDE, so I'm submitting it as a separate pull request.

[malsyned]

I think this change is a superset of #529 and contains all of those fixes.

[jerome-benoit]

The PR contains at least a serious fix to total_cost type, which is a showstopper at implementing an OCPP2 server compliant with pricing regulation with that library. The rest of PR is also refining properly a bunch of types definition.

[malsyned]

I rebased this so it will merge cleanly with master, and added the suggested additional type fix from @jerome-benoit. This should be good to go now.

[jerome-benoit]

@OrangeTux, @Jared-Newell-Mobility, @jainmohit2001: is there a chance that trivial PRs such as that one making the developer experience way more comfortable while enforcing strong types get reviewed and merged?

[jainmohit2001]

HI @malsyned, can you help with the following question?
Let's say the charger sends an Authorize request to the central system, we have the following handler in place:

@on(Action.Authorize)
async def on_authorize(self, id_token: dict, *args, **kwargs):
    status = AuthorizationStatusType.accepted
    return call_result.Authorize(id_token_info={"status": status})

Currently we have the following signature for Authorize dataclass:

@dataclass
class Authorize:
    id_token_info: Dict
    certificate_status: Optional[str] = None
    custom_data: Optional[Dict[str, Any]] = None

What would happen when we change the signature of Authorize to the following:

@dataclass
class Authorize:
    id_token: datatypes.IdTokenType
    certificate: Optional[str] = None
    iso15118_certificate_hash_data: Optional[List[datatypes.OCSPRequestDataType]] = None
    custom_data: Optional[Dict[str, Any]] = None

Do I need to make any changes to my @on handler?
How should I benefit from the new signature for Authorize dataclass?

[malsyned]

@jainmohit2001 I don't think you would have to make any changes, but I haven't looked at this in a while. I was working on an experimental @on-style decorator that uses these more specific type signatures to create ocpp objects, but as the library works now, the handler just receives a dictionary, and that shouldn't change because of this PR.

[ajmirsky]

@malsyned while i fully support including this pull request in the library as your changes now match the json schemas, it could be a breaking change, depending on how the caller of their handlers marshal the OCPP message into the payload.

@jainmohit2001
while mypy and IDEs with type checking will complain, no change is required if this is how the payloads gets built:

from ocpp.v201.call import BootNotification

def process_boot_notification(payload: BootNotification):
    vendor_name = payload.charging_station['vendor_name']


payload =  json.loads('{"charging_station": {"vendor_name": "OC", "model": "OC-X", "modem": {"iccid": "1234567890", "imsi": "1234567890"}, "serial_number": "123456", "firmware_version": "1.0.0"}, "reason": "PowerUp", "custom_data": null}')

boot_notification_payload = BootNotification(**payload)
type(boot_notification_payload.charging_station)  # <class 'dict'>

process_boot_notification(boot_notification_payload)

if the payload is hydrated recursively -- using a library such as dataclasses-json -- then there is a required change:

from ocpp.v201.call import BootNotification
from dataclasses_json.core import _decode_dataclass

def process_boot_notification(payload: BootNotification):
    vendor_name = payload.charging_station.vendor_name


payload =  json.loads('{"charging_station": {"vendor_name": "OC", "model": "OC-X", "modem": {"iccid": "1234567890", "imsi": "1234567890"}, "serial_number": "123456", "firmware_version": "1.0.0"}, "reason": "PowerUp", "custom_data": null}')

boot_notification_payload = _decode_dataclass(BootNotification, payload, infer_missing=False)

type(process_boot_notification.charging_station)  # <class 'ocpp.v201.datatypes.ChargingStationType'>
process_boot_notification(boot_notification_payload)

@jainmohit2001
the overall benefit is that mypy and IDEs will be able to catch errors prior to runtime as payload.charging_station['vendor_nm'] will not flag as an error until it throws a KeyError exception. if static type checking is in place, then payload.charging_station.vendor_nm would be caught ahead of time.

[jerome-benoit]

There's already release candidates in progress for the 2.0.0 release, the first version to include ocpp v2 support. And breaking changes in major revision rcs are totally fine. The only question remaining is: do the maintainers want to make the dev experience way more comfortable and strongly typed or not?
There's at least 2 PRs addressing that concern and totally fine at being included in a major revision rcs lifecycle.
Why the maintainers have not idenfied PRs needing to be included early in a major revision rcs lifecycle?
Why does it take so long to merge and do release candidate for sensitive PRs needing wider testing? Or to include hotfix? Especially for a project backed by a company.
The more it goes, the more I consider forking it to ease OCPP 2 integration and adoption.

[ajmirsky]

@malsyned i opened up a pull request against your branch to fix the linting errors and bring it up-to-date with base branch. hopefully this streamlines things instead of opening another pull request against the main fork.

[ajmirsky]

@jerome-benoit I just opened #684 to create a unit test that verifies no functionality changes happen with the nested dataclasses. I opened a new PR with the datatype changes #680 which includes the same unit test. could you review both and give feedback? the project recently changed ownership from company-backed to volunteer lead so I'm hopeful that the new maintainers will be able to address a few outstanding PRs.

@malsyned could you review the new PRs as well? would be great to have your feedback as well since you've been involved with the nested dataclass changes. is there anything else we can do to show that these aren't breaking changes?

[proelke]

Looks like there are some issues with Black. Can you update this PR?

[ajmirsky]

@proelke PR #680 is derived from this branch; it includes all of the Black fixes plus unit tests to prevent regression and two fixes to enum types.

[jainmohit2001]

Closing this PR as completed in #680