-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add rule and field value to violations #224
Conversation
protovalidate/validator.py
Outdated
|
||
def __init__(self, msg: str, violations: validate_pb2.Violations): | ||
def __init__(self, msg: str, violations: list[validate_pb2.Violations]): |
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.
something about the types here doesn't quite match up, we're passing in the list of proto messages (which is returned raw from def violations
) + _violations
is typed as list[Violation]
. I think they all need to match up?
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.
Yeah, you're right. It's unfortunate how easy it is to have extremely wrong types pass type checking. Oh well.
Should be fixed now. (It's a little awkward because Mypy doesn't support "aliasing" a type. Pylance/etc. don't seem to mind.)
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 good on my end, although my python is getting rustier all the time :). nothing blocking, just questions
Protobuf-specific collection type used by Violations. | ||
""" | ||
return list(self.violations.violations) | ||
return self._violations |
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 think this makes sense, because you've aliased _constraints.Violation
to Violation
; assuming that the constructor takes a _constraints.Violation
as it's supposed to be initialized internally, right?
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.
Yeah, this is supposed to return protovalidate.Violation
and not buf.validate.Violation
. It's probably a bit confusing due to the diff in the API but the naming scheme here now matches closer to the other runtimes.
ctx.add(Violation(constraint_id="unimplemented", message="Unimplemented")) | ||
|
||
|
||
class CelRunner: |
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.
could be a @dataclasses.dataclass
, I think?
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.
Yeah, this could just be a dataclass.
Adds the ability to access the captured rule and field value from a
Violation
.This is a breaking change. The API changes in the following ways:
ValidationError
has changed:ValidationError.violations
should callValidationError.to_proto()
instead, to get abuf.validate.Violations
message.ValidationError.errors
was removed. Switch to usingValidationError.violations
instead.ValidationError.violations
provides a list of the newViolation
wrapper type instead of a list ofbuf.validate.Violation
.Violation
wrapper type contains thebuf.validate.Violation
message under theproto
field, as well asfield_value
andrule_value
properties that capture the field and rule values, respectively.Validator.collect_violations
now operates on and returnslist[Violation]
instead of the protobufbuf.validate.Violations
message.This API mirrors the changes being made in protovalidate-go in bufbuild/protovalidate-go#154.