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

Improve messages in an array format #1498

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

henriquemoody
Copy link
Member

@henriquemoody henriquemoody commented Dec 18, 2024

Related to the discussion #1427

library/Result.php Outdated Show resolved Hide resolved
@henriquemoody henriquemoody force-pushed the core/path branch 3 times, most recently from cad83ce to 259be0c Compare December 20, 2024 23:55
Copy link

codecov bot commented Dec 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.53%. Comparing base (33e6341) to head (9b58ad8).

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #1498   +/-   ##
=========================================
  Coverage     96.53%   96.53%           
- Complexity      972      973    +1     
=========================================
  Files           201      201           
  Lines          2396     2397    +1     
=========================================
+ Hits           2313     2314    +1     
  Misses           83       83           

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

@henriquemoody henriquemoody force-pushed the core/path branch 6 times, most recently from b47c0b5 to d719129 Compare December 21, 2024 00:38
],
18 => [
'messages' => ['allOf' => '`false` must pass all the rules'],
'details' => ['arrayVal' => '`false` must be an array value'],
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this key because some rules have children but they're respective to the parent validation, not a child. I thought I would add an extra key there to identify those. I think that one would always use details instead of messages when it's available because messages would usually be only headers of nesting validations.

'messages' => ['each' => 'Each item in `["perm1": true, "perm2": false, "perm3": "boom!"]` must be valid'],
'children' => [
'perm3' => [
'messages' => ['boolVal' => '"boom!" must be a boolean value'],
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not replacing the input of each rule with the key, because I think that, since the structure is already in the array, it's best to keep the original message instead, so you can actually see the value that failed.

],
],
],
]
Copy link
Member Author

@henriquemoody henriquemoody Dec 27, 2024

Choose a reason for hiding this comment

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

I gotta say, we can added a whole bunch of other things here, I'm just not sure how they can be useful. Each element could contain the keys:

  • messages: all the messages that fails
  • details: more details about the messages that failed
  • children: all the children (which in turn could have all the keys I'm mentioning here
  • input: the input that was proceeded by the rule
  • path: the path of the rule that failed

But when I start to really think about this, I think this is not even an array in an exception anymore, it could be an object that contains all this information and handles all of them properly.

Maybe I'm just thinking of providing this array to the exception because of how the library worked before, but I could just drop the whole array thing, and return a proper object in Validator::validate() that would contain all the failed messages and the extra information that is needed -- maybe I'm just conditioned to how the library worked in the past.

Copy link
Member

Choose a reason for hiding this comment

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

I can offer some perspective on what I was thinking when I made the original library.

The array thing and a lot of things (like setting properties in runtime objects) were implementation details, mostly private stuff, that could be done in whatever way fits best (I decided to go for the solution that had lass code back then).

To me, the reportError (or was it reportException?) method was part of the validatable interface that could be extended and the thing I wanted to look nice. Together with the base lib, it offered two standard ways of creating exceptions:

  • ValidationException, which is a simple single message object.
  • AbstractNestedException, which is a more elaborate object, mostly used by aggregate validators, but actually an object of its own.

AbstractNestedException had things like findMessages with paths back then, although the workings were slightly different. Since each validator had to had a dedicated exception, I went for an abstract instead of a concrete named object like ValidationResults.

The decision to use exceptions also played a role in the design of Validatable::check, which I think doesn't exist anymore. It was designed to catch exceptions by type early instead of running the whole thing (the decision whether to fail fast or not). This complicated the internal workings of the project, and it was a compromise I'm not sure we still need to make. It seems our users don't care about fail fast that much.

So, you're still dealign with a lot of interlocked design decisions I made back then :D. You don't need to follow any of them if they don't make sense anymore. I would just go for whatever fullfills your vision best. That was what I did: I just moved things around until it was cohese.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants