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

feat: Pass field names to Validators and Constraints #263

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

nbaju1
Copy link

@nbaju1 nbaju1 commented Feb 6, 2025

This PR adds the ability for Validators and Constraints to access the field name they're validating. This enables more descriptive error messages and warnings that can reference the specific field being validated.

Changes

  • Added field_name property to Validator and Constraint classes
  • Pass field name through validation chain
  • Updated tests to verify field name and value type accessibility
  • Updated documentation

Use Case

This is particularly useful for constraints that need to reference the field name in their messages, such as deprecation warnings or field-specific validation errors.

This change allows Constraints to access the field name they're validating,
enabling more descriptive and context-aware error messages. This is particularly
useful for custom constraints that need to provide field-specific validation
messages.

Key changes:
- Pass field name from Schema._validate_item to Validator
- Add field_name property to Validator and Constraint classes
- Propagate field name through validation chain
- Update documentation with examples
README.md Outdated
@@ -446,6 +446,38 @@ Validates included structures. Must supply the name of a valid include.
Examples:
- `include('person')`

### Field Names in Constraints
Constraints can now access the field name they're validating, enabling more descriptive error messages. This is particularly useful for custom constraints that need to provide field-specific messages.
Copy link

@mildebrandt mildebrandt Feb 6, 2025

Choose a reason for hiding this comment

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

Consider changing to "Constraints can access the field name they're validating, enabling descriptive error messages". Removes the words "now" and "more".

Since this is a README, the text should indicate the feature has always been there....this text will be there for the next 10 years. :)

@@ -13,6 +13,8 @@ class Constraint(object):
is_active = False

def __init__(self, value_type, kwargs):
self.field_name = None # Added for custom field-aware messages
self.value_type = value_type # Added for custom type-aware messages

Choose a reason for hiding this comment

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

Consider creating a test for value_type.

@mildebrandt
Copy link

When I ran the tests, I got errors similar to the following:

>           validator.field_name = key                                                                                               
E           AttributeError: 'dict' object has no attribute 'field_name'                                                                                          
yamale/schema/schema.py:78: AttributeError                  

Did you get these errors when running the tests?

Unfortunately, not all validators are of the Validator class. They can also be dict, list, and probably others that escape my mind. If you wrap the setting of field_name in something like if isinstance(validator, val.Validator):, that should get you what you want.

@nbaju1
Copy link
Author

nbaju1 commented Feb 7, 2025

@mildebrandt, thanks for the review. Updated PR with your suggestions. Test suite wasn't properly setup on my end before creating the PR. Tests ran without errors for me now.

@nbaju1 nbaju1 requested a review from mildebrandt February 7, 2025 08:31
@nbaju1 nbaju1 force-pushed the feat/pass-field-names branch from ebf0e66 to 65963a5 Compare February 7, 2025 09:34
README.md Outdated
@@ -446,6 +446,38 @@ Validates included structures. Must supply the name of a valid include.
Examples:
- `include('person')`

### Field Names in Constraints

Choose a reason for hiding this comment

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

I don't think there's a case where we can have a custom constraint without a custom validator....so I think moving this under the Custom Validators section may be better. Please move it down and indent the heading by one...so 4 # signs.

@mildebrandt
Copy link

I think this looks good. But I have no power here anymore. :)

Someone from the team will want to review and decide if it's a feature that they want to include in Yamale.

@cblakkan cblakkan self-requested a review February 7, 2025 14:59
@nbaju1
Copy link
Author

nbaju1 commented Feb 21, 2025

@cblakkan, did you get a chance to look at this?

@sambowry
Copy link

sambowry commented Mar 1, 2025

I like the "deprecated" flag idea, but it would be more informative if it could be a message, not just a boolean value.

password: str(deprecated=True)
password: str(deprecated="The `password` field is deprecated. Please replace with SSO config")

@cblakkan
Copy link
Member

cblakkan commented Mar 1, 2025

I like the "deprecated" flag idea, but it would be more informative if it could be a message, not just a boolean value.

password: str(deprecated=True)
password: str(deprecated="The `password` field is deprecated. Please replace with SSO config")

I've been busy and haven't had a chance to dig into this yet but this was suggested to me by @joecackler and if done this way you would just need to write your own Constraint and there wouldn't need to be a change to Yamale.

This enables more descriptive error messages and warnings that can reference the specific field being validated.

One of the things I was going to dig into was how helpful the change might be in improving the existing validation error messages. Though if we do pass field_name and value_type to the Constraint it seems like it should be passed via the is_valid function as arguments instead of mutating the object.

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.

4 participants