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.

Sign up for GitHub

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

Jump to bottom

Rename DeviceModelStorage to DeviceModelInterface #768

Merged
merged 18 commits into from
Nov 14, 2024
Merged

Rename DeviceModelStorage to DeviceModelInterface #768

merged 18 commits into from
Nov 14, 2024

Conversation

maaikez
Copy link
Contributor

@maaikez maaikez commented Sep 3, 2024

Describe your changes

Rename 'DeviceModelStorage' to 'DeviceModelInterface'

Issue ticket number and link

#766

Checklist before requesting a review

@maaikez
Changes to the database and distinguish between external and ocpp val…
f108bf3 
…ues. Currently only the 'OCPP' source is accepted.

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
@maaikez
Make Variable Source a string instead of enum.
36471c2 
Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
@maaikez
Merge branch 'main' into feature/766-rename-devicemodelstorage-to-dev…
994694d 
…icemodelinterface
@maaikez
formatting...
910e270 
Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
@maaikez
Oops after merge
537daec 
Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
Pietfried
Pietfried approved these changes Sep 5, 2024
View reviewed changes
Copy link
Contributor

@Pietfried Pietfried left a comment

Choose a reason for hiding this comment

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

lgtm 👍

include/ocpp/v201/device_model.hpp Outdated
/// functionality to support the use cases defined in the functional block Provisioning
class DeviceModel {

private:
DeviceModelMap device_model;
std::unique_ptr<DeviceModelStorage> storage;
std::unique_ptr<DeviceModelInterface> interface;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would rather call this device_model_interface or just device_model and rename the
DeviceModelMap device_model to device_model_map

@Pietfried Pietfried linked an issue Sep 20, 2024 that may be closed by this pull request
Rename 'DeviceModelStorage' to 'DeviceModelInterface' #766
Closed
@marcemmers
Copy link
Contributor

I am actually implementing some tests that make use of the device model at the moment. That made me think if DeviceModelInterface is the right name for this? Since we might want to mock the DeviceModel itself in the future as well and then that should also get the name DeviceModelInterface right?

@maaikez
Copy link
Contributor Author

maaikez commented Nov 11, 2024

DeviceModelMock?

It is an interface isn't it? So what is your proposal then?

@marcemmers
Copy link
Contributor

Well the pattern we are using is ChargePoint implements a ChargePointInterface, following that the DeviceModel should implement the DeviceModelInterface if we are ever going to make unit tests for the device model. But now it actually is using the interface instead. So maybe DeviceModelStorageInterface? I am not sure what the reason was to go away from "Storage"?

@maaikez
Copy link
Contributor Author

maaikez commented Nov 11, 2024

Because it does not have to be a storage, if the implementer does not want to make it a storage (which is strange, I admit, but it might be runtime something).

@maaikez maaikez force-pushed the feature/766-rename-devicemodelstorage-to-devicemodelinterface branch from 325713a to 1c8ef1d Compare November 11, 2024 14:29
Pietfried
Pietfried reviewed Nov 11, 2024
View reviewed changes
config/v201/component_config/standardized/AlignedDataCtrlr.json Outdated
@@ -6,6 +6,7 @@
"properties": {
"AlignedDataCtrlrEnabled": {
"variable_name": "Enabled",
"source": "OCPP",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why we only have that im some of the files? To have an example?

Copy link
Contributor Author

@maaikez maaikez Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure anymore (it's a long time ago already), but I think so indeed.
I also don't know exactly which values should be for ocpp and which internal, from some I know but for the others we have to walk through them?

Copy link
Contributor

Choose a reason for hiding this comment

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

For now, all should be OCPP

Copy link
Contributor

Choose a reason for hiding this comment

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

Id suggest to remove the "source" here. It will be useful later when we add externally managed variables.

@maaikez
Remove source from component config (for now).
75dfaf1 
Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
@Pietfried Pietfried mentioned this pull request Nov 13, 2024
Merged
3 tasks
@maaikez
Bump version
2561f00 
Signed-off-by: Maaike Zijderveld, iolar <[email protected]>
@maaikez maaikez merged commit b00d278 into main Nov 14, 2024
7 of 8 checks passed
@maaikez maaikez deleted the feature/766-rename-devicemodelstorage-to-devicemodelinterface branch November 14, 2024 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pietfried Pietfried Pietfried approved these changes

@hikinggrass hikinggrass hikinggrass approved these changes

@marcemmers marcemmers Awaiting requested review from marcemmers marcemmers is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rename 'DeviceModelStorage' to 'DeviceModelInterface'
4 participants
@maaikez @marcemmers @hikinggrass @Pietfried