Skip to content

Conversation

@smaye81
Copy link
Contributor

@smaye81 smaye81 commented Jun 17, 2025

This adds the ability to specify a Config data class to validators when creating them. Prior to this PR, validators could be created one of two ways:

  • via a module-level singleton.
import protovalidate
protovalidate.validate(msg)
  • via explicit instantiation of the validator. i.e.
import protovalidate

validator = protovalidate.Validator()
validator.validate(msg)

With this PR, it is now possible to create a validator with a Config object, specifying various options (only fail_fast for now) for configuring a validator. To use a config, users must explicitly instantiate a validator and specify their config. They cannot use the module singleton as this only allows for a default validator.

For example:

import protovalidate
from protovalidate.config import Config

cfg = Config(fail_fast=True)
validator = protovalidate.Validator(config=cfg)

validator.validate(msg)

This PR is a breaking change. As a result of the above, the fail_fast parameters have been removed from the signatures for validate and collect_violations. Users should instead use the above config method to specify fail_fast.

This also adds a bit more depth to the validate_test unit tests by testing creation of a default validator via the module and via explicit instantiation. It also tests that the violations returned by collect_violations and raises as part of the exception returned from validate are the same.

@smaye81 smaye81 requested review from Alfus and stefanvanburen June 17, 2025 17:50
Copy link
Member

@stefanvanburen stefanvanburen left a comment

Choose a reason for hiding this comment

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

looks reasonable to me, two minor things

Comment on lines 260 to 265
assert str(vce.exception) == expected

# Test collect_violations
with self.assertRaises(protovalidate.CompilationError) as cvce:
v.collect_violations(msg)
assert str(cvce.exception) == expected
Copy link
Member

Choose a reason for hiding this comment

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

nit: use self.assertEqual to compare the exception strings? we're nearly fully on the unittest style of assertions, even though we're using the pytest runner, so makes sense to be consistent. 🤷

validate = _default_validator.validate
collect_violations = _default_validator.collect_violations

__all__ = ["CompilationError", "ValidationError", "Validator", "Violations", "collect_violations", "validate"]
Copy link
Member

Choose a reason for hiding this comment

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

do we need to from config import Config and add it to __all__ here so that consumers can use it?

Copy link
Member

Choose a reason for hiding this comment

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

(eh, I guess not if we just do the from protovalidate.config import Config, ignore me maybe)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'm not sure if there's a benefit or not? I can definitely add it if you think it's worth it.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's somewhat common to include everything that's "public" re-exported in the __init__.py, so I think it's reasonable. so that callers can do:

from protovalidate import Validator, Config

instead of

from protovalidate import Validator
from protovalidate.config import Config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, updated. i had to keep all the stuff internal to the protovalidate module though to use from protovalidate.config, otherwise i was seeing this:

 ImportError: cannot import name 'Config' from partially initialized module 'protovalidate' (most likely due to a circular import)

Unless you see something obvious I'm missing?

Copy link
Member

Choose a reason for hiding this comment

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

that seems right, yep

@smaye81 smaye81 merged commit 0c7db6c into main Jun 17, 2025
12 checks passed
@smaye81 smaye81 deleted the sayers/add_config branch June 17, 2025 19:05
stefanvanburen added a commit that referenced this pull request Aug 26, 2025
stefanvanburen added a commit that referenced this pull request Aug 26, 2025
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.

3 participants