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

Remove OperationVariable class #148

Closed
zrgt opened this issue Oct 24, 2023 · 8 comments
Closed

Remove OperationVariable class #148

zrgt opened this issue Oct 24, 2023 · 8 comments
Labels
BlockingRelease This issue needs to be solved before the pending release v3.0

Comments

@zrgt
Copy link
Contributor

zrgt commented Oct 24, 2023

  • Use SubmodelElement instead of OperationVariable in Operation class
  • Remove OperationVariable class
@zrgt zrgt mentioned this issue Oct 24, 2023
Closed
@zrgt
Copy link
Contributor Author

zrgt commented Oct 24, 2023

@jkhsjdhjs Can you take care of the issue?

@jkhsjdhjs jkhsjdhjs mentioned this issue Oct 24, 2023
Closed
26 tasks
@s-heppner s-heppner added v3.0 BlockingRelease This issue needs to be solved before the pending release labels Oct 29, 2023
jkhsjdhjs added a commit to rwth-iat/basyx-python-sdk that referenced this issue Nov 3, 2023
@jkhsjdhjs
make OperationVariable a Namespace
d4a2c6e 
This commit makes `OperationVariable` inherit from
`UniqueIdShortNamespace`, to implement Constraint AASd-134:

For an Operation, the idShort of all inputVariable/value,
outputVariable/value, and inoutputVariable/value shall be unique.

In the DotAAS spec, the attributes `inputVariable`, `outputVariable`
and `inoutputVariable` of `Operation` are defined to be a collection of
`OperationVariable` instances, which themselves just contain a single
`SubmodelElement`. Thus, the `OperationVariable` isn't really required
for `Operation`, as the `Operation` can just contain the
`SubmodelElements` directly, without an unnecessary wrapper. This makes
`Operation` less tedious to use and also allows us to use normal
`NamespaceSets` for the 3 attributes, which together with the
`UniqueIdShortNamespace` ensure, that the `idShort` of all contained
`SubmodelElements` is unique across all 3 attributes.

Aside this, the examples are updated since `SubmodelElements` as
children of an `Operation` are now linked to the parent. This prevents
us from reusing other `SubmodelElements` as `OperationVariables` as it
was done previously, since each `SubmodelElement` can only have one
parent.

Fix eclipse-basyx#146 eclipse-basyx#148
@jkhsjdhjs jkhsjdhjs mentioned this issue Nov 3, 2023
Merged
jkhsjdhjs added a commit to rwth-iat/basyx-python-sdk that referenced this issue Nov 5, 2023
@jkhsjdhjs
make OperationVariable a Namespace
d0cf3d5 
This commit makes `OperationVariable` inherit from
`UniqueIdShortNamespace`, to implement Constraint AASd-134:

For an Operation, the idShort of all inputVariable/value,
outputVariable/value, and inoutputVariable/value shall be unique.

In the DotAAS spec, the attributes `inputVariable`, `outputVariable`
and `inoutputVariable` of `Operation` are defined to be a collection of
`OperationVariable` instances, which themselves just contain a single
`SubmodelElement`. Thus, the `OperationVariable` isn't really required
for `Operation`, as the `Operation` can just contain the
`SubmodelElements` directly, without an unnecessary wrapper. This makes
`Operation` less tedious to use and also allows us to use normal
`NamespaceSets` for the 3 attributes, which together with the
`UniqueIdShortNamespace` ensure, that the `idShort` of all contained
`SubmodelElements` is unique across all 3 attributes.

Aside this, the examples are updated since `SubmodelElements` as
children of an `Operation` are now linked to the parent. This prevents
us from reusing other `SubmodelElements` as `OperationVariables` as it
was done previously, since each `SubmodelElement` can only have one
parent.

Fix eclipse-basyx#146 eclipse-basyx#148
@zrgt
Copy link
Contributor Author

zrgt commented Nov 7, 2023

We may have to reconsider the decision to eliminate the OperationVariable class due to possible problems with DotAAS version updates. If new attributes are added to OperationVariable in the future, we will be forced to build the class again and integrate those attributes. Such changes in the future will also require changes to our users' code and impact the backwards compatibility.

@jkhsjdhjs , @s-heppner , @TorbenD what do you think?

@zrgt
Copy link
Contributor Author

zrgt commented Nov 7, 2023

Unfortunately if we want to keep OperationVariable class we will have to revert some changes made in d0cf3d5

To solve #146 we may introduce a new property submodel_element_id_short in OperationVariable and do smth like this in Operation.__init__:

self.input_variable = base.NamespaceSet(self, [("submodel_element_id_short", True)], input_variable)

@jkhsjdhjs
Copy link
Contributor

Such changes in the future will also require changes to our users' code and impact the backwards compatibility.

Good point. On the other hand adding attributes to OperationVariable would require a new version, and seeing how breaking changes are introduced even in release candidates, I doubt there won't be any in the next minor version. Thus you could also argue, that we might as well remove OperationVariable, because one breaking change more or less will probably not make a big difference.

@TorbenD
Copy link
Contributor

TorbenD commented Nov 8, 2023

Good points. How about the integration into the server-implementation which will come next? Maybe it would be easier to still remain with the OperationVariable for less adaption in other applications based on the sdks in future.

jkhsjdhjs added a commit to rwth-iat/basyx-python-sdk that referenced this issue Nov 8, 2023
@jkhsjdhjs
make Operation a Namespace
a21b5cb 
This commit makes `Operation` inherit from
`UniqueIdShortNamespace`, to implement Constraint AASd-134:

For an Operation, the idShort of all inputVariable/value,
outputVariable/value, and inoutputVariable/value shall be unique.

In the DotAAS spec, the attributes `inputVariable`, `outputVariable`
and `inoutputVariable` of `Operation` are defined to be a collection of
`OperationVariable` instances, which themselves just contain a single
`SubmodelElement`. Thus, the `OperationVariable` isn't really required
for `Operation`, as the `Operation` can just contain the
`SubmodelElements` directly, without an unnecessary wrapper. This makes
`Operation` less tedious to use and also allows us to use normal
`NamespaceSets` for the 3 attributes, which together with the
`UniqueIdShortNamespace` ensure, that the `idShort` of all contained
`SubmodelElements` is unique across all 3 attributes.

Aside this, the examples are updated since `SubmodelElements` as
children of an `Operation` are now linked to the parent. This prevents
us from reusing other `SubmodelElements` as `OperationVariables` as it
was done previously, since each `SubmodelElement` can only have one
parent.

Fix eclipse-basyx#146 eclipse-basyx#148
@zrgt
Copy link
Contributor Author

zrgt commented Nov 8, 2023

Such changes in the future will also require changes to our users' code and impact the backwards compatibility.

Good point. On the other hand adding attributes to OperationVariable would require a new version, and seeing how breaking changes are introduced even in release candidates, I doubt there won't be any in the next minor version. Thus you could also argue, that we might as well remove OperationVariable, because one breaking change more or less will probably not make a big difference.

The removing of OperationVariable class would still affect the backwards compatibility.

@jkhsjdhjs
Copy link
Contributor

Sure, my argument is that there are breaking changing with every new version of the metamodel, attributes are renamed all the time or are of a different type in a new version. So if we can't expect much backwards compatibility for other classes anyway, is it worth keeping OperationVariable just for a bit more backwards compatibility? And that is assuming OperationVariable changes in the future, which we don't know yet.

s-heppner pushed a commit that referenced this issue Nov 14, 2023
@jkhsjdhjs @s-heppner
make Operation a Namespace
1d87d2d 
This commit makes `Operation` inherit from
`UniqueIdShortNamespace`, to implement Constraint AASd-134:

For an Operation, the idShort of all inputVariable/value,
outputVariable/value, and inoutputVariable/value shall be unique.

In the DotAAS spec, the attributes `inputVariable`, `outputVariable`
and `inoutputVariable` of `Operation` are defined to be a collection of
`OperationVariable` instances, which themselves just contain a single
`SubmodelElement`. Thus, the `OperationVariable` isn't really required
for `Operation`, as the `Operation` can just contain the
`SubmodelElements` directly, without an unnecessary wrapper. This makes
`Operation` less tedious to use and also allows us to use normal
`NamespaceSets` for the 3 attributes, which together with the
`UniqueIdShortNamespace` ensure, that the `idShort` of all contained
`SubmodelElements` is unique across all 3 attributes.

Aside this, the examples are updated since `SubmodelElements` as
children of an `Operation` are now linked to the parent. This prevents
us from reusing other `SubmodelElements` as `OperationVariables` as it
was done previously, since each `SubmodelElement` can only have one
parent.

Fix #146 #148
@s-heppner
Copy link
Contributor

Fixed in #151

Frosty2500 pushed a commit to rwth-iat/basyx-python-sdk that referenced this issue Aug 22, 2024
@jkhsjdhjs @s-heppner
make Operation a Namespace
bf52267 
This commit makes `Operation` inherit from
`UniqueIdShortNamespace`, to implement Constraint AASd-134:

For an Operation, the idShort of all inputVariable/value,
outputVariable/value, and inoutputVariable/value shall be unique.

In the DotAAS spec, the attributes `inputVariable`, `outputVariable`
and `inoutputVariable` of `Operation` are defined to be a collection of
`OperationVariable` instances, which themselves just contain a single
`SubmodelElement`. Thus, the `OperationVariable` isn't really required
for `Operation`, as the `Operation` can just contain the
`SubmodelElements` directly, without an unnecessary wrapper. This makes
`Operation` less tedious to use and also allows us to use normal
`NamespaceSets` for the 3 attributes, which together with the
`UniqueIdShortNamespace` ensure, that the `idShort` of all contained
`SubmodelElements` is unique across all 3 attributes.

Aside this, the examples are updated since `SubmodelElements` as
children of an `Operation` are now linked to the parent. This prevents
us from reusing other `SubmodelElements` as `OperationVariables` as it
was done previously, since each `SubmodelElement` can only have one
parent.

Fix eclipse-basyx#146 eclipse-basyx#148
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
BlockingRelease This issue needs to be solved before the pending release v3.0
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jkhsjdhjs @TorbenD @zrgt @s-heppner