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 Constraint AASd-133 in SpecificAssetId #147

Merged

Conversation

zrgt
Copy link
Contributor

@zrgt zrgt commented Oct 20, 2023

Check if external_subject_id is ExternalReference

fixes #145

Check if external_subject_id is ExternalReference
@jkhsjdhjs
Copy link
Contributor

This constraint was already enforced by the typechecker, as we declare the type as ExternalReference. Not sure if we should implement this exception anyway.

@s-heppner
Copy link
Contributor

Also, you probably mean #145 don't you?

@s-heppner
Copy link
Contributor

This constraint was already enforced by the typechecker, as we declare the type as ExternalReference. Not sure if we should implement this exception anyway.

I think we should check it explicitly, since the type checker only analyzes the code statically, whereas this implementation also ensures correctness during runtime.

@s-heppner
Copy link
Contributor

Looks good to me. I'll rebase if you say it's ready.

@zrgt
Copy link
Contributor Author

zrgt commented Nov 2, 2023

@s-heppner it's ready

@jkhsjdhjs
Copy link
Contributor

I think we should check it explicitly, since the type checker only analyzes the code statically, whereas this implementation also ensures correctness during runtime.

We should stay consistent though. We don't verify any other types because the typechecker does it for us. Everything that can be verified by the typechecker is currently only verified by the typechecker and not at runtime. If a variable is declared as a certain type, we always assume that the variable is of the declared type. Why shouldn't we do it in this case?

basyx/aas/model/base.py Outdated Show resolved Hide resolved
@s-heppner
Copy link
Contributor

We decided not to implement the check for reasons of consistency. However, we leave the documentation that this constraint exists.

@s-heppner s-heppner merged commit 5eb895e into eclipse-basyx:improve/V30 Nov 3, 2023
@s-heppner s-heppner deleted the add/Constraint_AASd-133 branch November 3, 2023 10:40
@s-heppner s-heppner added the v3.0 label Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants