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

v2.0.1 Action enums corrected #580

Merged
merged 18 commits into from
Feb 14, 2024

Conversation

Jared-Newell-Mobility
Copy link
Collaborator

See issue #579

(cherry picked from commit 30b69d3)
@Jared-Newell-Mobility Jared-Newell-Mobility added bug Something isn't working For inclusion in release 1.0.0 and removed bug Something isn't working labels Jan 17, 2024
@Jared-Newell-Mobility Jared-Newell-Mobility marked this pull request as draft January 17, 2024 15:50
@OrangeTux
Copy link
Collaborator

OrangeTux commented Jan 25, 2024

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

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

@Jared-Newell-Mobility
Copy link
Collaborator Author

Jared-Newell-Mobility commented Jan 26, 2024

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

  • Action("BootNotification") results in
    <Action.BootNotification: 'BootNotification'>
  • Action.BootNotification.name results in
    'BootNotification'
  • Action.BootNotification.value results in
    'BootNotification'
  • Action.BootNotification results in
    <Action.BootNotification: 'BootNotification'>
  • Action.boot_notification results in
    <Action.BootNotification: 'BootNotification'>
  • Action.boot_notification.name results in
    'BootNotification'
  • Action.boot_notification.value results in
    'BootNotification'

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:

  • catch a member only when in it missing but in our case it is replaced
  • if we could make use of it, it will works in this case when accessing via Action("BootNotification") but not Action.BootNotification.name (as this will no longer exist after removal)

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?

@Jared-Newell-Mobility Jared-Newell-Mobility marked this pull request as ready for review January 26, 2024 09:56
@OrangeTux
Copy link
Collaborator

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

Jared-Newell-Mobility commented Feb 2, 2024

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

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.

Copy link
Collaborator

@OrangeTux OrangeTux left a comment

Choose a reason for hiding this comment

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

I realized that the enums of v16 are also incorrect. Can you update those as well?

@Jared-Newell-Mobility Jared-Newell-Mobility mentioned this pull request Feb 6, 2024
26 tasks
@@ -2,6 +2,9 @@
- [#564](https://github.com/mobilityhouse/ocpp/issues/564) Add support For Python 3.11 and 3.12
- [#583](https://github.com/mobilityhouse/ocpp/issues/583) OCPP v1.6/v2.0.1 deprecate dataclasses from calls and call results with the suffix 'Payload'

## BREAKING ##
Copy link
Collaborator

Choose a reason for hiding this comment

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

These changes are not yet breaking, I suggest to change the title to "Deprecated".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Satisfied with commit acba23f

warn(
"Action enum contains deprecated members and will be removed in "
"the next major release, please use snake case members."
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you apply the comment I made here to this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Satisfied with commit 50e4a95

@OrangeTux
Copy link
Collaborator

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

OrangeTux
OrangeTux previously approved these changes Feb 13, 2024
@Jared-Newell-Mobility Jared-Newell-Mobility merged commit 144b544 into master Feb 14, 2024
6 checks passed
@Jared-Newell-Mobility Jared-Newell-Mobility deleted the v2.0.1-Action-enums-corrected branch February 14, 2024 13:35
Jared-Newell-Mobility added a commit that referenced this pull request Apr 25, 2024
- OCPP v1.6/v2.0.1 deprecate dataclasses from calls and call results with the suffix 'Payload' by @Jared-Newell-Mobility in #584
- In GA, validate project against Python 3.11 and 3.12. by @Jared-Newell-Mobility in #589
- drop support for python 3.7 by @Jared-Newell-Mobility in #585
- Update Code Owners by @Jared-Newell-Mobility in #588
- Revert "drop support for python 3.7" by @Jared-Newell-Mobility in #597
- OCPP 2.0.1 Wrong data type in CostUpdated total_cost by @Jared-Newell-Mobility in #596
- Update tests to use Call and CallResult without the suffix Payload by @Jared-Newell-Mobility in #595
- Fix camel_to_snake_case for "v2x" by @Jared-Newell-Mobility in #594
- Correct naming of members of v201.enums.AttributeType by @Jared-Newell-Mobility in #578
- Drop support for python 3.7 by @Jared-Newell-Mobility in #598
- Remove v1.6 deprecated enum variants by @Jared-Newell-Mobility in #575
- Typo in CostUpdated Action #435 by @Jared-Newell-Mobility in #491-
- Remove support for ocpp 2.0 by @Jared-Newell-Mobility in #576
- v201.datatypes.ChargingNeedsType.request_energy_transfer is mistyped by @Jared-Newell-Mobility in #496
- v201.enums.StatusInfoReasonType.invaild_schedule by @Jared-Newell-Mobility in #521
- update to match Appendix 2. Standardized Units Of Measure by @Jared-Newell-Mobility in #512
- v16/schemas/StopTransaction.json missing "Hertz" #207 by @Jared-Newell-Mobility in #497
- Correct v2g serialisation/deserialisation by @Jared-Newell-Mobility in #606
- 2.0.1 dataclasses have a incorrect types that don't match carnality by @Jared-Newell-Mobility in #529
- Readthedocs_configuration_is_outdated by @Jared-Newell-Mobility in #608
- Readthedocs_configuration_is_outdated_config_update by @Jared-Newell-Mobility in #609
- The serialisation of soc to SoC should not occur in camel case if it is existing at the beginning of a field by @Jared-Newell-Mobility in #527
- Fix case conversion for soc in non "State of Charge" context by @Jared-Newell-Mobility in #607
- Handle recursively serializing a dataclasses as a dictionary. by @Jared-Newell-Mobility in #547
- v2.0.1 Action enums corrected by @Jared-Newell-Mobility in #580
- URL does not get converted from snake_case responder_url to camelCase responderURL by @Jared-Newell-Mobility in #592
- v1.6 Action Enum members corrected by @Jared-Newell-Mobility in #600
- Introduce Experimental Module For v2.1 by @Jared-Newell-Mobility in #605
- Bump to 1.0.0-rc.1 by @Jared-Newell-Mobility in #611
- fix typo in DataTypeEnum -> value_too_high by @d2avids in #612
- fix typo CostUpdated enum for 201 by @OSkrk in #620
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants