[sc-154361] #3 Implement evaluation of rules, prerequisites, and fallthrough. #14
Conversation
|
This pull request has been linked to Shortcut Story #155754: Implement Remaining Evaluation. |
kinyoklion
changed the base branch from
rlamb/sc-154361/target-evaluation
to
rlamb/sc-155755/implement-reviver-replacer
| attribute: 'key', | ||
| op: 'in', | ||
| values: [user.key], | ||
| attributeReference: new AttributeReference('key'), |
There was a problem hiding this comment.
Since attributeReference is an implementation detail of the preprocessing logic, might it be better to expose the function for doing preprocessing on a Clause so that test code like this could just set the regular properties and then call that? That way, the only code that needs to know about the extra attributeReference property is 1. the code that does the preprocessing and 2. the evaluation logic, not random test helpers like this.
| return this.detail.reason.kind === Reasons.Off.kind; | ||
| } | ||
|
|
||
| static ForError(errorKind: ErrorKinds, message?: string): EvalResult { |
There was a problem hiding this comment.
Is it normal in TS for methods like this to have an initial capital?
There was a problem hiding this comment.
This is probably an older C# static method naming convention bleeding in. I've always liked it, but I am not sure if it would be common or uncommon in TS generally speaking. Looks like there isn't a linting rule one way or the other, which is unexpected for the very opinionated linter.
There was a problem hiding this comment.
I can change them though, I have absolute indifference.
There was a problem hiding this comment.
All methods in C# are supposed to have an initial capital, no? Isn't that just a basic difference in naming conventions between C# and JavaScript, rather than anything specifically about it being static?
There was a problem hiding this comment.
C# C++
Regardless they are not capitalized any longer.
| * | ||
| * @internal | ||
| */ | ||
| export default function matchClause(clause: Clause, context: Context): boolean { |
There was a problem hiding this comment.
It might be best to give this a slightly more specific name (as we've done in some other SDKs) to indicate that it doesn't implement the segmentMatch operator. Then matchClause could be defined to include the "do segment stuff if it is segmentMatch" behavior - we will want that to be reusable, because both flag rule clauses and segment rule clauses can reference segments.
| import * as http from 'http'; | ||
| import * as https from 'https'; | ||
|
|
||
| // No types for the event source. |
There was a problem hiding this comment.
These were not ready yet.
This PR implements the evaluation of rules/clauses, prerequisites, and fallthrough.
It includes tests for rules/clauses, but does not include prerequisite tests. I will add those once there is support for the prerequisite events.