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

Allow Min, Max, and Each to wrap chained rules. #1501

Closed
wants to merge 5 commits into from

Conversation

dmjohnsson23
Copy link
Contributor

Following the pattern set in #1485, allow, the Min, Max, and Each rules to elegantly support chained rules with well-formatted validation messages.

This follows the same pattern as `Length`.

I've also removed the separate named and standard templates from these rules. I didn't see a good way to implement these in the new pattern, and also felt the language of the old standard template was somewhat difficult to read anyway.

This also contains a small update to a few of the test helper functions to prevent issues when running tests on systems with different line endings.
@dmjohnsson23
Copy link
Contributor Author

I've done Min and Max. I've also removed the separate named and standard templates from these rules. I didn't see a good way to implement these in the new pattern, and also felt the language of the old standard template was somewhat difficult to read anyway.

Still looking at Each. I think it may require a slightly different pattern than the others. I'm looking through the codebase to get a better idea of how the formatter actually works before I tackle that one.

This also contains a small update to a few of the test helper functions to prevent issues when running tests on systems with different line endings.

Copy link

codecov bot commented Dec 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.57%. Comparing base (74c018b) to head (cde9fa0).
Report is 3 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1501      +/-   ##
============================================
+ Coverage     96.56%   96.57%   +0.01%     
- Complexity      978      979       +1     
============================================
  Files           201      202       +1     
  Lines          2417     2426       +9     
============================================
+ Hits           2334     2343       +9     
  Misses           83       83              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Dominick Johnson added 3 commits December 19, 2024 14:13
…class.

This base class could also be used for other aggregate operations on arrays, e.g. a sum. (It can't be used for `Length` though, as we would not be able to validate a length of 0.)
@dmjohnsson23
Copy link
Contributor Author

I'm not actually sure what needs to be done for Each. I added a few tests as a way to evaluate the current behavior and see what needed changed, and I actually was able to get the tests to pass without making any changes to the Each rule itself. So, there may be something I'm still not understanding.

@henriquemoody, do the tests I added in the most recent commit reflect intended behavior, or is there something that needs changed? The only things that seemed like they might be incorrect to me are:

  • The shortened error message uses the first leaf in the tree of messages, rather than the root. However, I'm assuming this is intended behavior, as the test you added for Length enforces the same behavior.
  • In the array structure, "allOf" branches collapse down to a single string when there is only one error, rather than remaining an array. The full message does not exhibit this same behavior, so this seems like it may not be the intended behavior to me.

@henriquemoody
Copy link
Member

Hey, @dmjohnsson23 !

Thank you so much for your changes!

What I would expect from when v::each() is called with a single rule would be something like this:

v::each(v::intType())->assert([1, 2, 3, 'B']);
// Message: All items in `[1, 2, 3, 'B']` must be an integer type

But that's quite complicated, and I'm not sure if I want to replicate the same behaviour from other rules, if I'm honest. I will think a bit better about that, and I will let you know.

I created #1502 with your changes, but with atomic commits and changing the commits slightly to look like the other commits in the repo. I've noticed that your email is not configured properly, so I changed your author to Dominick Johnson <[email protected]>, I hope you don't mind?

@dmjohnsson23
Copy link
Contributor Author

That's fine, thank you

@henriquemoody
Copy link
Member

Closed by #1502

Thank you so much for contributing, @dmjohnsson23!

@henriquemoody
Copy link
Member

Thank you! Much appreciated 🐼

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.

2 participants