-
Notifications
You must be signed in to change notification settings - Fork 15
[RFC14] Part 3: Define constraints inside the value type #667
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
Conversation
libs/execution/src/lib/types/value-types/value-representation-validity.ts
Outdated
Show resolved
Hide resolved
libs/language-server/src/lib/ast/wrappers/value-type/atomic-value-type.ts
Outdated
Show resolved
Hide resolved
| ConstraintDefinition | ||
| ValueTypeConstraintInlineDefinition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Future work: this could return a constraint wrapper instead
props, | ||
); | ||
if (isValueTypeConstraintInlineDefinition(constraint)) { | ||
checkConstraintExpression(constraint.expression, props); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we check somewhere that the attribute we refer to in the inline constraint exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we don't, but neither for inline definitions nor external definitions.
The code snippets
valuetype VT {
property id oftype text;
constraint exactlyTenCharacters: lengthof unknown == 10;
}
valuetype VT {
property id oftype text;
constraint len: ExactlyTenCharacters on id;
}
constraint ExactlyTenCharacters on text: lengthof unknown == 10;
both fail during execution with the same message
Could not resolve reference to Referencable named 'unknown'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to catch this in a validation, so as a language server hint. But can be postponed to a follow-up PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I'll already approve the PR, so you can merge at your own pace.
Please make sure that at least the validation function I commented on is in place before merging ;-)
Final part of implementing RFC14.
Continues #665.
This PR allows for constraints to be defined inside a value type.
Example of the new syntax: