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

Add v2.0.1 data types #240

Merged
merged 5 commits into from
Sep 16, 2021
Merged

Conversation

proelke
Copy link
Collaborator

@proelke proelke commented Aug 23, 2021

Adding the various datatypes for v2.0.1 as I didn't see them in the repo already.

@proelke proelke requested a review from OrangeTux as a code owner August 23, 2021 22:22
attribute_value: str
component: ComponentType
variable: VariableType
attribute_type: Optional[enums.AttributeType] = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Even if Auke was not sure on adding the custom data field, it is by default None, so it wont impact nothing if we add it and it will make it in sync with the schemas

Copy link
Collaborator

@tropxy tropxy left a comment

Choose a reason for hiding this comment

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

Just a minor comment regarding CustomDataType

@proelke proelke force-pushed the add_v201_datatypes branch from 4dce8c0 to cd58077 Compare August 25, 2021 12:40
Comment on lines 95 to 96
full_SoC: Optional[int] = None
bulk_SoC: Optional[int] = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Abbreviations in attribute names should be lower case. See for example ev_max_current.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was worried these wouldn't convert correctly with the weird format but I will test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In testing when converting to camel case the C is lowercase. I am not sure how you want to approach this edge case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your point. However, using the current implementation of snake_to_camel_case and camel_to_snake_case the conversion of bulk_SoC works only one way: from snake case to camel case.

If a CP sends a DCChargingStationParameterType to a CSMS, the CSMS will convert bulkSoC to bulk_so_c.

I suggest to introduce a map than explicitly maps bulkSoC to bulk_soc and vice versa. The functions camel_to_snake_case and snake_to_camel_case have to use this map first, before applying the regex.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I took a slightly different approach and included some test cases. Let me know what you think.

Copy link
Contributor

@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.

Thanks!

@OrangeTux OrangeTux merged commit 1051320 into mobilityhouse:master Sep 16, 2021
ajmirsky pushed a commit to ajmirsky/ocpp that referenced this pull request Nov 26, 2024
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