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

refactor: predicate lang #7851

Merged
merged 1 commit into from Jun 5, 2023
Merged

refactor: predicate lang #7851

merged 1 commit into from Jun 5, 2023

Conversation

rgrinberg
Copy link
Member

Individual constructors for true/false.

This gives a more compact representation for these common forms.

Signed-off-by: Rudi Grinberg me@rgrinberg.com

Individual constructors for true/false.

This gives a more compact representation for these common forms.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

<!-- ps-id: 350c2ebf-35aa-4998-8b66-02f37f372e9c -->
@@ -94,6 +96,8 @@ and decode f = many f or_ []
let rec encode f =
let open Encoder in
function
| True -> encode f (Or [])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems odd: I'd expect something like True -> encode f True here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you make a typo? That would just be an infinite loop then.

It is odd, but I'm afraid it's unavoidable as we don't have a way to represent true or false as literals in the predicate language. We could introduce :true and :false for example though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, I meant mapping True and False to the corresponding constants in the predicate language, like :true and :false, for example.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking is: if we allow (or ()) in the language, we better also allow (:true) for clarity.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but this is actually a change that needs versioning. On the rules side, we must only accept this starting from 3.9. On the engine side, would we need to bump the action digest version because we aren't serializing Predicate_lang.true_ in the same way?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, we can do this later, of course.

Copy link
Collaborator

@snowleopard snowleopard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@rgrinberg rgrinberg merged commit 1106c6c into main Jun 5, 2023
41 checks passed
@rgrinberg rgrinberg deleted the ps/rr/refactor__predicate_lang branch June 5, 2023 13:11
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.

None yet

2 participants