-
Notifications
You must be signed in to change notification settings - Fork 12
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
Implement expressions #290
Conversation
Includes the comparison of numeric and text values.
libs/language-server/src/lib/ast/expressions/operators/binary-arithmetic-expression.ts
Outdated
Show resolved
Hide resolved
libs/language-server/src/lib/ast/expressions/operators/binary-arithmetic-expression.ts
Outdated
Show resolved
Hide resolved
libs/language-server/src/lib/validation/checks/property-assignment.ts
Outdated
Show resolved
Hide resolved
libs/language-server/src/lib/validation/checks/property-assignment.ts
Outdated
Show resolved
Hide resolved
libs/language-server/src/lib/validation/checks/property-assignment.ts
Outdated
Show resolved
Hide resolved
libs/language-server/src/lib/validation/checks/property-assignment.ts
Outdated
Show resolved
Hide resolved
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.
Good job on the refactoring!
From a code review perspective it looks good to me, but can't guarantee that I was able to catch all bugs (esp. because the feature is quite large). I feel like we should start introducing tests soon with the increasing logic.
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.
Sick 👍
libs/execution/src/lib/constraints/executors/allowlist-constraint-executor.ts
Show resolved
Hide resolved
@@ -25,18 +26,16 @@ export class TextFileInterpreterMetaInformation extends BlockMetaInformation { | |||
}, | |||
validation: (property, context) => { | |||
const propertyValue = property.value; | |||
assert(isTextLiteral(propertyValue)); | |||
assert(isExpression(propertyValue)); |
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.
This seems to be repeated quite a bit, might be good to have a util method to get property values of a specific type. I.e. evaluatedPropertyExpression<T>
(in this case evaluatedPropertyExpression<string>
that reads the property, evaluates the expression, asserts the type and returns the correctly typed result.
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.
Preparation for #213, implementation of RFC 0009
Introduces new operators, type inference and evaluation semantics for expressions. Also includes options for
lazy
andexhaustive
evaluation (affects binary boolean operators so they don't necessarily evaluate both sub-expressions).In many cases, the code for type inference is pretty much the same per operator, except for some special cases (e.g. different treatment of
decimal
andinteger
and that operands of==
/!=
need to have the same type). I'm not sure how to properly refactor the code, so both usual and special cases can be covered conveniently. Introducing a sub-type relationship betweeninteger
anddecimal
or an implicit conversion mechanism could already help to generally cope with the first special case. Let me know if you have further ideas, maybe we can discuss that in a call.The prior redundancy in the evaluation was mostly eliminated in 02f05bd by introducing evaluator classes that implement shared evaluation logic. A similar refactoring for type inference also address the problem described above.
Known issue:
Sometimes, when using symbolic operators without surrounding spaces, a lexer error occurs:
Not sure why that is the case (probably due to the grammar?). I fiddled around with it a bit but didn't find an immediate solution for the problem.