-
Notifications
You must be signed in to change notification settings - Fork 775
Ensure formatters only format, not modify results #1572
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a new Subject class to encapsulate the relationship between Name, Path, and input values, consolidating data that was previously tracked separately in the Result class. This refactoring simplifies state management and provides a cleaner API for tracking validation subjects.
Key changes:
- Created new
Subjectclass to holdinput,path,name, andisNamePrinproperties together - Refactored
Resultclass to useSubjectinstead of separateinput,path, andnameproperties - Updated all file references to access these properties through
result->subject
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| library/Subject.php | New class that encapsulates input, path, name, and related state with immutable methods for creating modified copies |
| library/Result.php | Refactored to use Subject instead of individual properties; simplified methods like withPath, withName, and introduced withSubject |
| library/Name.php | Simplified by removing Path dependency and the withPath method; removed unused import |
| library/Rules/UndefOr.php | Updated to access input through result->subject->input |
| library/Rules/NullOr.php | Updated to access input through result->subject->input |
| library/ResultQuery.php | Updated to access name and path through result->subject |
| library/Message/Stringifier/NameStringifier.php | Refactored to work with Subject instead of Name for stringification |
| library/Message/InterpolationRenderer.php | Simplified parameter extraction using Subject; added Subject import |
| library/Message/Formatter/TemplateResolver.php | Updated to access path and name through result->subject |
| library/Message/Formatter/NestedListStringFormatter.php | Refactored to pass displayedName through method parameters and access name via result->subject->name |
| library/Message/Formatter/NestedArrayFormatter.php | Updated to access path through result->subject->path |
| library/Message/Formatter/FirstResultStringFormatter.php | Simplified by removing the parent name tracking logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d21aefd to
25cdef8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b11c7d4 to
c09b9a3
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1572 +/- ##
============================================
- Coverage 97.54% 97.50% -0.05%
+ Complexity 999 995 -4
============================================
Files 211 213 +2
Lines 2280 2282 +2
============================================
+ Hits 2224 2225 +1
- Misses 56 57 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c09b9a3 to
1b9f7e8
Compare
Subject to track pairs of Name and PathThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
811a9bb to
6d59764
Compare
When I changed the library to not overwrite existing names [1], I wasn't happy with how `FirstResultStringFormatter` was changing the results, because the results should be completely ready by the time they arrive in the formatters. This commit changes that behaviour, ensuring the results are complete with all necessary information by the time they reach the formatters. Along with those changes, I refactored some stringifiers and simplified the `InterpolationRenderer`; we were not overwriting the "name" parameter anyway, as it was just an unnecessary overhead. [1] 8332d28
6d59764 to
9a30ce5
Compare
When I changed the library to not overwrite existing names 1, I wasn't happy with how
FirstResultStringFormatterwas changing the results, because the results should be completely ready by the time they arrive in the formatters.This commit changes that behaviour, ensuring the results are complete with all necessary information by the time they reach the formatters.
Along with those changes, I refactored some stringifiers and simplified the
InterpolationRenderer; we were not overwriting the "name" parameter anyway, as it was just an unnecessary overhead.