Unit test covering all v2.0.1 datatypes and enums #684
Conversation
Also includes a pull request template. Contributes to mobilityhouse#671
…changes made to nested dataclasses don't affect functionality
ajmirsky
changed the title
…pe. eliminates breaking change.
ajmirsky
force-pushed
the
v201_datatypes_test
branch
2 times, most recently
from
ea02518 to
e53d73c
Compare
SECURITY.md
Outdated
| | Version | Supported | | ||
| |----------| ------------------ | | ||
| | 2.0.0 | :white_check_mark: | | ||
| | 0.26.0 | :white_check_mark: | |
There was a problem hiding this comment.
No version 1.x.x has been released?
There was a problem hiding this comment.
not quite yet : (
there's a release candidate pending since february: https://github.com/mobilityhouse/ocpp/releases
probably should remove 2.0.0 from this list too, until we have a release candidate for that one too
There was a problem hiding this comment.
apologies for the confusion, i removed these files as they are really part of #681
CONTRIBUTING.md
Outdated
| 1. [Fork][fork] and clone the repository. | ||
| 1. Create a new branch: `git checkout -b my-branch-name`. | ||
| 1. Configure and install the dependencies: `poetry install`. | ||
| 1. Make sure the tests pass on your machine: `make install & make tests` |
There was a problem hiding this comment.
A migration to task should be considered. make is not really a build tool the python community use.
There was a problem hiding this comment.
i would agree... sort of. make is so ubiquitous; I've seen many (legitimate) uses of make and python.
for the time being with this PR, was trying to document current process : )
if you think it's important, i could get behind it. could you create an issue ticket so that the community can have the discussion about it?
There was a problem hiding this comment.
apologies for the confusion, i removed these files as they are really part of #681
There was a problem hiding this comment.
@jerome-benoit which task library were you referring to? there's pytask. but also snakemake, scons, invoke, bitbake all of which are python friendly, although probably way more functionality than is needed here.
There was a problem hiding this comment.
taskpy. The Makefile is only used here to run ordered shell command(s), so is pointless. taskpy can do the same without requiring another external tool.
CONTRIBUTING.md
Outdated
| 1. Make your change, add tests, and make sure the tests still pass. | ||
| 1. Push to your fork and [submit a pull request][pr] and complete the information in the pull request template. | ||
|
|
||
| ## Linting requirements |
There was a problem hiding this comment.
A migration to ruff should be considered.
There was a problem hiding this comment.
i'm not as familiar with ruff as black, isort and flake8. could you open an issue and give some justification? i'd be happy to switch over if there's enough support from the community.
apologies for the confusion, i removed these files as they are really part of #681. could you add addition comments there?
| @@ -1364,6 +1387,11 @@ class UnitOfMeasureType(StrEnum): | |||
| k = "K" | |||
|
|
|||
|
|
|||
| UnitOfMeasureType = DeprecatedEnumWrapper( | |||
There was a problem hiding this comment.
Not sure it worth the effort. Have these enums been part of any stable release.
Considering rc versions as API stable is unusual.
There was a problem hiding this comment.
i'm mixed on this one.
correct, there has been no official stable release.
But... since 0.26.0 has been in the wild for the past year, there are definitely people who are using/depending on the library
I leaned towards usability for the community in this case but I could be convinced otherwise.
There was a problem hiding this comment.
@jerome-benoit actually, given that these OCPP 2.0 files are included in this library's 1.0 release (on pypi), I think it's better to be safe. I don't think the project has made it clear if the library's release version is tied to the OCPP version or not?
There was a problem hiding this comment.
The library version is totally independent from the OCPP version supported.
| @@ -454,7 +454,7 @@ class IdTokenType: | |||
| """ | |||
|
|
|||
| id_token: str | |||
| type: enums.IdTokenType | |||
| type: enums.IdTokenEnumType | |||
There was a problem hiding this comment.
Not sure what's the value add of renaming enums
There was a problem hiding this comment.
Compliance with the OCPP specs
There was a problem hiding this comment.
Cross Checked this. This makes me question the whether the rest of the enums and datatypes are compliant or not. Will take a look at this soon.
There was a problem hiding this comment.
I did a double check when I came across this one and all of the others seemed compliant. But another set of eyes is always appreciated!
|
closing out in favor of the tests in #684 |
Changes included in this PR
Adding unit test for all of the v2.0.1 datatypes, so that if any changes -- namely nested models -- are done, there are no regressions in functionality.
Impact
Two enum types were changed to eliminate confusion with their datatype counterparts:
IdTokenTypeis nowIdTokenEnumType(matches its name in the json schema)UnitOfMeasureTypeis nowStandardizedUnitsOfMeasureType(matches the name from OCPP 2.0 Appendix 2)To not cause any breaking changes,
IdTokenTypeandUnitOfMeasureTypeare aliased to their new counterparts, with a warning message added upon access of their enum members. This method was used as enums can't be subclassed with a__post_init__method as are some of the datatypes in v1.6.Checklist
Existing tests pass (with no changes needed). Adding test coverage with no change in functionality