-
Notifications
You must be signed in to change notification settings - Fork 9
Pass --strict_message conformance (minus duration/timestamp)
#256
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
Conversation
We can't currently pass the conformance tests involving duration+timestamp types, as cel-python does not support nanoseconds in its duration implementation (linked issue). Otherwise, this gets us to a conformant spot. Related to #255.
| return celtypes.StringType(arg.hex()) | ||
| return celtypes.StringType(arg) | ||
| if isinstance(arg, celtypes.ListType): | ||
| return self.format_list(arg) | ||
| if isinstance(arg, celtypes.BoolType): | ||
| # True -> true | ||
| return celtypes.StringType(str(arg).lower()) | ||
| if isinstance(arg, celtypes.DoubleType): | ||
| return celtypes.StringType(f"{arg:.0f}") | ||
| if isinstance(arg, celtypes.TimestampType): | ||
| base = arg.isoformat() | ||
| if arg.getMilliseconds() != 0: | ||
| base = arg.isoformat(timespec="milliseconds") | ||
| return celtypes.StringType(base.removesuffix("+00:00") + "Z") |
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.
just for context, most protovalidate rules are defined in CEL using the %s verb (in fact, I don't think I've seen another verb used). format_string handles that verb, so we need to add some additional handling here.
| if isinstance(arg, celtypes.DurationType): | ||
| return celtypes.StringType(f'duration("{arg.seconds + (arg.microseconds // 1e6):.0f}s")') | ||
| if isinstance(arg, celtypes.DoubleType): | ||
| return celtypes.StringType(f"{arg:f}") |
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.
format_value handles lists, where we add more context to what type of value is in the list (think repr instead of str in Python). hence the additional context.
|
Also for my future context: the definition of |
Still issues with tests using below-microsecond nanosecond values, but these tests use millisecond values, so we can fix them up.
|
Last note for myself and others: cel-python doesn't support nanosecond resolution in Going to merge this now as I think it's a step in the right direction; I do think that we want to more fully test |
We can't currently pass the conformance tests involving duration+timestamp types, as cel-python does not support nanoseconds in its duration implementation (linked issue). Otherwise, this gets us to a conformant spot.
Related to #255.