- Notifications
You must be signed in to change notification settings - Fork36
Optional support... in the parser#137
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Draft
alexsnaps wants to merge2 commits intocel-rust:masterChoose a base branch fromalexsnaps:optional
base:master
Could not load branches
Branch not found:{{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline, and old review comments may become outdated.
Draft
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
Signed-off-by: Alex Snaps <alex@wcgw.dev>
Signed-off-by: Alex Snaps <alex@wcgw.dev>
Openeda discussion on a related subject. |
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading.Please reload this page.
These few commits add support for the optional syntax (
?
) in the parser and AST nodes. Now, as it turns out, the golang implementation hasatype Optional struct
, whichimplementstype Val interface
...So I can either "hack" that in the current interpreter (i.e. probably short-circuit interpretation, and "early" return
Null
), or... I guess it's what I'd recommend anyways, I start revisiting the whole type system. Which also leads me to bunch of questions. Including the need forValue
to own "thread safe" values... I never really got why this is dealt with at that level in the "graph". As a CEL expression is to be immutable (modulo some cases e.g. the accumulator in macros), and even if an expression is to be shared across thread for concurrent execution, wouldn't it be theContext
that should be thread safe, and can't we consider the expression as immutable, possibly the expression being reference counted?Anyways, I guess I'm opening quite a can of worms here. But I think I'd be tempted to start exploring revisiting the type system, leave thread-safety aside, if only for now. There, my first question becomes: should we go for
dyn
amic dispatch (i.e. have a it all model with traits) or would it be nice to keep things as concrete as possible? I feel the go implementation has a bit of "interface derangement syndrome", but it might also all be for a good reason...