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

model.base: Remove occurrences of deprecated constraint AASd-100 #121

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

s-heppner
Copy link
Contributor

@s-heppner s-heppner commented Sep 1, 2023

Version 3.0 of the specification removes AASd-100:

Since new string types with length constraints were added,
this constraint is no longer needed
Constraint AASd-100: An attribute with data type "string"
is not allowed to be empty

@s-heppner s-heppner added the v3.0 label Sep 1, 2023
@s-heppner
Copy link
Contributor Author

@jkhsjdhjs Can you review this?

:param unit: unit of the data object (optional)
:raises ValueError: if the constraint is not fulfilled
"""
if unit == "":
raise ValueError("unit is not allowed to be an empty string")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep these ValueErrors as we otherwise have an ambiguity with None and an empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This issue will be solved automatically, when implementing AASd-130 (#118) :

Constraint AASd-130: An attribute with data type "string" shall consist of these
characters only: ^[\x09\x0A\x0D\x20-\uD7FF\uE000-\uFFFD\u00010000-
\u0010FFFF]*$.

Copy link
Contributor

Choose a reason for hiding this comment

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

How? As far as I can tell this regex allows the empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, but in combination with the constrained string type regex, it should not be allowed. (Otherwise that's another finding)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but unit, sourceOfDefinition and symbol are not of any constrained string type, but simply of type string.
But I also just noticed that while Constraint AASd-100 seens to have been removed from the DataSpecification classes, it is still part of Part1, where it doesn't make any sense anymore, since Part1 doesn't contain any attributes of type string. But assuming it still applies to Part3a, should this be left as is or simply changed to an AASConstraintViolation(100, ...) instead of a ValueError()?

@s-heppner s-heppner merged commit 18fde95 into eclipse-basyx:improve/V30 Sep 8, 2023
5 checks passed
@s-heppner s-heppner deleted the Remove/AASd-100 branch September 8, 2023 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants