mobilityhouse / ocpp

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

v2.0.1 Action enums corrected #580

Closed Jared-Newell-Mobility closed 6 months ago

Jared-Newell-Mobility commented 7 months ago

See issue https://github.com/mobilityhouse/ocpp/issues/579

OrangeTux commented 7 months ago

I'm unsure about this PR. Strictly speaking, this PR is correct. The PR corrects the casing the Action enum to be consistent with the rest.

However, the change is backwards compatible and will break almost every application. What do you think of allowing the old notation (e.g. GetVariables) for now, but de deprecate them and remove them in a v2 version of ocpp. The deprecation warning should urge users to use the new notation (e.g get_variables).

Python doesn't support deprecating enum members natively. But someone on StackOverflow came up with a clever fix using the __missing__() dunder method.

Jared-Newell-Mobility commented 7 months ago

I completely agree with you here, I will modify this PR and see if we incorporate the warning

Jared-Newell-Mobility commented 7 months ago

I have added the previous members back into the Action and in their current order, the result is as follows:

The problem we have here is that the name of the members changes and the value remains the same. Due to this the member is never missing, given the constraints of missing (see below).

So after looking into this using missing we can only:

So to introduce a deprecation warning and also to remove these into the future becomes quite a conundrum ... however, the current change could be a logical step?

OrangeTux commented 7 months ago

Not sure if I follow you.

I did some tests and figured out that _missing_() is only called when you instantiate an enum like Action("BootNotification").

Normal lookups, like Action.BootNotification, won't call _missing_() and therefore won't print a warning.

I fixed that with this fix.

from warnings import warn
from enum import StrEnum

class Action(StrEnum):
    BootNotification = "BootNotification"
    boot_notification = "BootNotification"

    def __init__(self, *args, **kwargs):
        if self.name == 'BootNotification':
            warn("Action.BootNotification is deprecated and will be removed in the future. Please use Action.boot_notification.")

        return super().__init__(*args, **kwargs)

print(Action.BootNotification)
print(Action.BootNotification.name)
print(Action.BootNotification.value)
print(Action.boot_notification)
print(Action.boot_notification.name)
print(Action.boot_notification.value)
print(Action['boot_notification'])
print(Action("BootNotification"))

It prints:

/tmp/dep.py:10: UserWarning: Action.BootNotification is deprecated and will be removed in the future. Please use Action.boot_notification.
  warn("Action.BootNotification is deprecated and will be removed in the future. Please use Action.boot_notification.")
Jared-Newell-Mobility commented 7 months ago

I tried the above approach using __init__ but the warning is only happens once during initialisation of the enum and doesn't appear during its use:

PyDev console: starting.
Python 3.11.7 (main, Dec  8 2023, 18:56:58) [GCC 11.4.0] on linux
from test_enums import Action
/home/jared/PycharmProjects/test_project/test_enums.py:11: UserWarning: Action.BootNotification is deprecated and will be removed in the future. Please use Action.boot_notification.
  warn(

Action.BootNotification
<Action.BootNotification: 'BootNotification'>
Action.boot_notification
<Action.BootNotification: 'BootNotification'>
Action.boot_notification.value
'BootNotification'
Action.boot_notification.name
'BootNotification'

So I attempted to modify this some more and got this:

from warnings import warn
from enum import StrEnum

class Action(StrEnum):

    def __init__(self):
        if self._name_ == "BootNotification":
            self._value_ = self._name_
        if self._name_ == "boot_notification":
            self._value_ = self._name_.title().replace("_","")

    BootNotification = ()
    boot_notification = ()

    @classmethod
    def _missing_(cls, value):
        if value == "BootNotification":
            warn(
                "Action.BootNotification is deprecated and will be removed in the future. Please use Action.boot_notification.")
            return cls.BootNotification

which results in:

from test_enums import Action

Action("BootNotification")
<Action.BootNotification: 'BootNotification'>
/home/jared/PycharmProjects/test_project/test_enums.py:18: UserWarning: Action.BootNotification is deprecated and will be removed in the future. Please use Action.boot_notification.
  warn(
Action.BootNotification.name
'BootNotification'
Action.BootNotification.value
'BootNotification'
Action.boot_notification.name
'BootNotification'
Action.boot_notification.value
'BootNotification'
Action.BootNotification
<Action.BootNotification: 'BootNotification'>
Action.boot_notification
<Action.BootNotification: 'BootNotification'>

Although this solves some of the problem, I couldn't get the new enum boot_notification to function correctly. I have identified that the order of members in the enums looks to determine this behaviour ....

OrangeTux commented 7 months ago

Good catch. I didn't realize the warning is show, even if you don't use any deprecated variants.

I'm afraid we can't use the builtin warning to warn the users.

OrangeTux commented 7 months ago

You can forget my earlier comment to include the v16 action. I see that the v16 action in #600 .