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

breaking change: remove deprecated Action items and update docs #694

Merged
merged 9 commits into from
Dec 19, 2024

Conversation

drc38
Copy link
Contributor

@drc38 drc38 commented Dec 9, 2024

Changes included in this PR

Breaking change
As per the warning in 1.0.0 this PR removes deprecated Action items from the enums.py files to coincide with the 2.0.0 release, updates code references and updates documentation to be consistent with the change including string references.

For example:

  • replaces deprecated Action enums such as Action.BootNotification with Action.boot_notification
  • replaces string references such as @on("BootNotification") and action="BootNotification" with Action.boot_notification

If users of the library have not already migrated to the new Action enums they will need to do so before using 2.0.0.

Checklist

  1. Does your submission pass the existing tests?
  2. Are there new tests that cover these additions/changes?
  3. Have you linted your code locally before submission?

@drc38 drc38 requested review from mdwcrft and proelke as code owners December 9, 2024 19:48
@drc38 drc38 marked this pull request as draft December 9, 2024 19:54
@drc38 drc38 marked this pull request as ready for review December 9, 2024 22:15
@drc38 drc38 mentioned this pull request Dec 9, 2024
Copy link
Contributor

@ajmirsky ajmirsky left a comment

Choose a reason for hiding this comment

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

given the name (and purpose) of the function _raise_key_error, standard checking using if/else makes more sense. if using try/except blocks, then we should just remove this function and add this try/except to the calling code.

@jainmohit2001
Copy link
Collaborator

jainmohit2001 commented Dec 16, 2024

Need to update the following:

  • docs/source/central_system.rst. Replace Action.BootNotification with Action.boot_notification.
  • examples/v16/central_system.py. Replace Action.BootNotification with Action.boot_notification.
  • Update all the comments and docs mentioned in ocpp/routing.py where Action. keyword is present.
  • examples/v201/central_system.py: Replace @on("BootNotification") and @on("Heartbeat") with respective Action enums.
  • tests/test_exceptions.py: Replace action="BootNotification" with relevant Action enum.
  • tests/v201/conftest.py: Replace action="BootNotification" and action="Heartbeat" with relevant Action enums.
  • tests/test_messages.py: Replace action="Heartbeat" with relevant Action enum.

Let's create another PR for the doc changes. For the rest, let's update them in this PR itself.

@drc38
Copy link
Contributor Author

drc38 commented Dec 16, 2024

Need to update the following:

  • docs/source/central_system.rst. Replace Action.BootNotification with Action.boot_notification.
  • examples/v16/central_system.py. Replace Action.BootNotification with Action.boot_notification.
  • Update all the comments and docs mentioned in ocpp/routing.py where Action. keyword is present.
  • examples/v201/central_system.py: Replace @on("BootNotification") and @on("Heartbeat") with respective Action enums.
  • tests/test_exceptions.py: Replace action="BootNotification" with relevant Action enum.
  • tests/v201/conftest.py: Replace action="BootNotification" and action="Heartbeat" with relevant Action enums.
  • tests/test_messages.py: Replace action="Heartbeat" with relevant Action enum.

Let's create another PR for the doc changes. For the rest, let's update them in this PR itself.

Just noticed your comment on a separate PR for doc changes, I've included them all in the above commit. Hopefully that's ok as then when this is merged all Action references are consistent.

@jainmohit2001
Copy link
Collaborator

Need to update the following:

  • docs/source/central_system.rst. Replace Action.BootNotification with Action.boot_notification.
  • examples/v16/central_system.py. Replace Action.BootNotification with Action.boot_notification.
  • Update all the comments and docs mentioned in ocpp/routing.py where Action. keyword is present.
  • examples/v201/central_system.py: Replace @on("BootNotification") and @on("Heartbeat") with respective Action enums.
  • tests/test_exceptions.py: Replace action="BootNotification" with relevant Action enum.
  • tests/v201/conftest.py: Replace action="BootNotification" and action="Heartbeat" with relevant Action enums.
  • tests/test_messages.py: Replace action="Heartbeat" with relevant Action enum.

Let's create another PR for the doc changes. For the rest, let's update them in this PR itself.

Just noticed your comment on a separate PR for doc changes, I've included them all in the above commit. Hopefully that's ok as then when this is merged all Action references are consistent.

No worries. We can go with the doc changes in this PR itself.

@jainmohit2001
Copy link
Collaborator

jainmohit2001 commented Dec 16, 2024

LGTM. @drc38, can you please update the description for the PR and include relevant changes done in this PR.
Also, please include the fact that this is a breaking change and the impact this will have.
Thanks!

@drc38 drc38 changed the title remove deprecated Action items breaking change: remove deprecated Action items and update docs Dec 16, 2024
@jainmohit2001
Copy link
Collaborator

HI @drc38, can you please rebase this with the master and update datatypes/enums used (if any).

@drc38
Copy link
Contributor Author

drc38 commented Dec 18, 2024

HI @drc38, can you please rebase this with the master and update datatypes/enums used (if any).

Master has been merged and tests are still passing.

@jainmohit2001 jainmohit2001 merged commit d25580c into mobilityhouse:master Dec 19, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants