-
-
Notifications
You must be signed in to change notification settings - Fork 432
Refactor expressions #715
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 expressions #715
Conversation
…s-experiment * origin/learn-the-rules: (57 commits) Add threadsafe: option to memory adapter Ensure memory adapter unlocked on fork Simplify boolean enabled Make expressions work with nil or empty array of actors Fix expression gate for no actor Add some actor specs a bit of formatting Get rid of shotgun Release 0.28.0 Update changelog Add a few more multiple actor specs Add example with multiple actors and in different order Another type spot A couple minor type tweaks Just add multiple actors on Pass flipper_ids so we can make that supported on the backend in cloud flipper_id should be string Less change to log subscriber Fix benchmark language Show example of deprecated payload argument ...
jnunemaker
left a comment
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.
Looks great. I left a few minor things.
My only concern is the time stuff. I think it's too liberal right now for multi-language. We'll probably want to lock it down and only accept strings in a particular format or formats.
| module Expressions | ||
| class Time | ||
| def self.call(value) | ||
| ::Time.parse(value) |
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 will assume local time, not utc, won't it? I stumbled on this stack overflow when googling.
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.
Only if the string passed in excludes the timezone. In the schemas I'm working on, I explicitly defined Time to take a date-time, which JSON Schema defines as an ISO8601 formatted string. Here are the tests:
Time
valid
✓ {"Time":"2021-01-01T00:00:00Z"}
✓ {"Time":["2021-01-01T00:00:00Z"]}
✓ {"Time":"2021-01-01T00:00:00-05:00"}
✓ {"Time":["2021-01-01T00:00:00-05:00"]}
✓ {"Time":{"Property":"created_at"}}
✓ {"Time":[{"Property":"created_at"}]}
invalid
✓ {"Time":"2021-01-01"}
✓ {"Time":"January 1, 2021 10:00"}
✓ {"Time":null}
✓ {"Time":false}
✓ {"Time":[{"Property":"created_at"},{"Property":"updated_at"}]}
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.
I should add I'm planning to pull those schemas in and validate the input in Expression.build.
Note some of the breaking changes in more detail.
Co-authored-by: John Nunemaker <[email protected]>
Fix typo in code comment
* origin/main: Fix typo in comment Update Changelog.md
…s-experiment * origin/learn-the-rules:
This is a fairly significant refactor of Expressions (#692).
Constant(which quacks like anExpression) to represent explicit string, number, and boolean values. Note that there are stillString,Number, andBooleanexpressions, which will perform casting of values passed to them, but those expressions are no longer used by default to represent constant values when serialized:{ "Equal" => [ {"Property" => ["plan"]}, - {"String" => ["basic"]}, + "basic", ] } ```#calland receives any arguments instead of subclassingExpression. All evaluating of arguments is done internally inExpressionbefore being passed to the#callmethod. If thecontext:key is part of the method signature, it will be passed in.This greatly simplifies the process of adding new expressions internally, and paves the way for making it easy for users to create expressions of their own.