-
Notifications
You must be signed in to change notification settings - Fork 770
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
Use paths to identify when a rule fails #1507
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
henriquemoody
commented
Dec 22, 2024
henriquemoody
commented
Dec 22, 2024
henriquemoody
commented
Dec 22, 2024
henriquemoody
commented
Dec 22, 2024
henriquemoody
commented
Dec 22, 2024
henriquemoody
commented
Dec 22, 2024
henriquemoody
commented
Dec 22, 2024
henriquemoody
commented
Dec 22, 2024
henriquemoody
commented
Dec 22, 2024
henriquemoody
force-pushed
the
core/result-path
branch
2 times, most recently
from
December 22, 2024 06:01
1d8934d
to
a4c83a7
Compare
henriquemoody
force-pushed
the
core/result-path
branch
3 times, most recently
from
December 27, 2024 00:52
882e18a
to
fdf13fd
Compare
henriquemoody
commented
Dec 27, 2024
henriquemoody
force-pushed
the
core/result-path
branch
from
December 27, 2024 04:05
341730d
to
c518582
Compare
henriquemoody
commented
Dec 27, 2024
henriquemoody
commented
Dec 27, 2024
henriquemoody
force-pushed
the
core/result-path
branch
3 times, most recently
from
December 27, 2024 15:55
2ca49bb
to
41fee0f
Compare
henriquemoody
commented
Dec 27, 2024
henriquemoody
force-pushed
the
core/result-path
branch
2 times, most recently
from
December 27, 2024 20:33
89b9d8e
to
cccd28d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1507 +/- ##
============================================
+ Coverage 96.64% 96.69% +0.04%
- Complexity 999 1014 +15
============================================
Files 205 205
Lines 2477 2511 +34
============================================
+ Hits 2394 2428 +34
Misses 83 83 ☔ View full report in Codecov by Sentry. |
henriquemoody
force-pushed
the
core/result-path
branch
3 times, most recently
from
December 27, 2024 21:57
6085889
to
661ce71
Compare
henriquemoody
force-pushed
the
core/result-path
branch
from
December 27, 2024 22:01
661ce71
to
50cfd46
Compare
henriquemoody
changed the title
Trace paths instead of IDs
Use paths to identify when a rule fails
Dec 27, 2024
When nested-structural validation fails, it's challenging to identify which rule failed from the main exception message. A great example is the `Issue796Test.php` file. The exception message says: host must be a string But you're left unsure whether it's the `host` key from the `mysql` key or the `postgresql` key. This commit changes that behaviour by introducing the concept of "Path." The `path` represents the path that a rule has taken, and we can use it in structural rules to identify the path of an array or object. Here's what it looks like before and after: ```diff -host must be a string +`.mysql.host` must be a string ``` Because paths are a specific concept, I added a dot (`.`) at the beginning of all paths when displaying them. I was inspired by the `jq` syntax. I also added backticks around paths to distinguish them from any other value. I didn't manage to fix a test, and I skipped it instead of fixing it because I want to make changes in how we display error messages as arrays, and it will be easier to fix it then.
henriquemoody
force-pushed
the
core/result-path
branch
from
December 27, 2024 22:28
50cfd46
to
1915b6f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When nested-structural validation fails, it's challenging to identify which rule failed from the main exception message. A great example is the
Issue796Test.php
file. The exception message says:host must be a string
But you're left unsure whether it's the
host
key from themysql
key or thepostgresql
key.This commit changes that behaviour by introducing the concept of "Path." The
path
represents the path that a rule has taken, and we can use it in structural rules to identify the path of an array or object.Here's what it looks like before and after:
Because paths are a specific concept, I added a dot (
.
) at the beginning of all paths when displaying them. I was inspired by thejq
syntax. I also added backticks around paths to distinguish them from any other value.I didn't manage to fix a test, and I skipped it instead of fixing it because I want to make changes in how we display error messages as arrays, and it will be easier to fix it then.