-
-
Notifications
You must be signed in to change notification settings - Fork 176
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: support context in errors formatter #554
Conversation
WalkthroughThe changes across the codebase enhance error handling and validation processes by integrating context awareness. A new variable, Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #554 +/- ##
==========================================
- Coverage 92.76% 90.67% -2.09%
==========================================
Files 22 22
Lines 3883 4033 +150
==========================================
+ Hits 3602 3657 +55
- Misses 236 286 +50
- Partials 45 90 +45 ☔ View full report in Codecov by Sentry. |
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.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- error.go (1 hunks)
- huma.go (2 hunks)
- validate.go (20 hunks)
- validate_test.go (3 hunks)
Additional context used
GitHub Check: codecov/patch
validate.go
[warning] 200-200: validate.go#L200
Added line #L200 was not covered by tests
[warning] 208-208: validate.go#L208
Added line #L208 was not covered by tests
[warning] 216-216: validate.go#L216
Added line #L216 was not covered by tests
[warning] 225-225: validate.go#L225
Added line #L225 was not covered by tests
[warning] 243-243: validate.go#L243
Added line #L243 was not covered by tests
[warning] 252-252: validate.go#L252
Added line #L252 was not covered by tests
[warning] 260-260: validate.go#L260
Added line #L260 was not covered by tests
[warning] 268-268: validate.go#L268
Added line #L268 was not covered by tests
[warning] 277-277: validate.go#L277
Added line #L277 was not covered by tests
[warning] 286-286: validate.go#L286
Added line #L286 was not covered by tests
[warning] 294-294: validate.go#L294
Added line #L294 was not covered by tests
[warning] 302-302: validate.go#L302
Added line #L302 was not covered by tests
[warning] 310-310: validate.go#L310
Added line #L310 was not covered by tests
[warning] 318-318: validate.go#L318
Added line #L318 was not covered by tests
[warning] 341-341: validate.go#L341
Added line #L341 was not covered by tests
[warning] 361-361: validate.go#L361
Added line #L361 was not covered by tests
[warning] 383-383: validate.go#L383
Added line #L383 was not covered by tests
[warning] 461-461: validate.go#L461
Added line #L461 was not covered by tests
[warning] 476-476: validate.go#L476
Added line #L476 was not covered by tests
[warning] 512-512: validate.go#L512
Added line #L512 was not covered by tests
[warning] 531-531: validate.go#L531
Added line #L531 was not covered by tests
[warning] 540-540: validate.go#L540
Added line #L540 was not covered by tests
[warning] 549-549: validate.go#L549
Added line #L549 was not covered by tests
[warning] 558-558: validate.go#L558
Added line #L558 was not covered by tests
[warning] 571-571: validate.go#L571
Added line #L571 was not covered by tests
[warning] 582-582: validate.go#L582
Added line #L582 was not covered by tests
[warning] 591-591: validate.go#L591
Added line #L591 was not covered by tests
[warning] 600-603: validate.go#L600-L603
Added lines #L600 - L603 were not covered by tests
[warning] 618-618: validate.go#L618
Added line #L618 was not covered by tests
[warning] 655-655: validate.go#L655
Added line #L655 was not covered by tests
Additional comments not posted (4)
error.go (1)
369-371
: Addition ofErrorFormatterContext
is approved.The introduction of
ErrorFormatterContext
provides a flexible mechanism for context-aware error message formatting while maintaining backward compatibility.validate.go (1)
Line range hint
186-319
: Context-aware validation logic is well-implemented.The addition of the
Context
parameter and the use ofErrorFormatterContext
enhance the flexibility of error message formatting.Tools
GitHub Check: codecov/patch
[warning] 200-200: validate.go#L200
Added line #L200 was not covered by tests
[warning] 208-208: validate.go#L208
Added line #L208 was not covered by tests
[warning] 216-216: validate.go#L216
Added line #L216 was not covered by tests
[warning] 225-225: validate.go#L225
Added line #L225 was not covered by tests
[warning] 243-243: validate.go#L243
Added line #L243 was not covered by tests
[warning] 252-252: validate.go#L252
Added line #L252 was not covered by tests
[warning] 260-260: validate.go#L260
Added line #L260 was not covered by tests
[warning] 268-268: validate.go#L268
Added line #L268 was not covered by tests
[warning] 277-277: validate.go#L277
Added line #L277 was not covered by tests
[warning] 286-286: validate.go#L286
Added line #L286 was not covered by tests
[warning] 294-294: validate.go#L294
Added line #L294 was not covered by tests
[warning] 302-302: validate.go#L302
Added line #L302 was not covered by tests
[warning] 310-310: validate.go#L310
Added line #L310 was not covered by tests
[warning] 318-318: validate.go#L318
Added line #L318 was not covered by testsvalidate_test.go (1)
1410-1450
: New tests for context-aware error formatting are well-implemented.The
TestValidateContextFormatter
andBenchmarkValidateContext
functions effectively validate and benchmark the context-aware error formatting.huma.go (1)
1146-1146
: Modification to useValidateContext
is approved.The use of
ValidateContext
in theRegister
function enhances the validation process by leveraging context for request-scoped values.Also applies to: 1254-1254
if ErrorFormatterContext == nil { | ||
res.Add(path, v, validation.MsgExpectedMatchExactlyOneSchema) | ||
} else { | ||
res.Add(path, v, ErrorFormatterContext(ctx, validation.MsgExpectedMatchExactlyOneSchema)) |
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.
Consider adding test coverage for context-aware error formatting.
The static analysis hints indicate that some lines involving ErrorFormatterContext
are not covered by tests.
Would you like me to help generate tests to cover these lines or open a GitHub issue to track this task?
Also applies to: 361-361, 383-383, 461-461, 476-476, 512-512, 531-531, 540-540, 549-549, 558-558, 571-571, 582-582, 591-591, 600-603, 618-618, 655-655
Tools
GitHub Check: codecov/patch
[warning] 341-341: validate.go#L341
Added line #L341 was not covered by tests
@smacker I had a look at this and played with it locally a bit. It's a good start, but we are still precomputing schema validation messages using |
Thanks @danielgtaylor! |
Hello!
This is a follow up for #520 to allow localization of the error messages.
My idea is that we can't assume any particular implementation for the localization, instead we can provide an interface that would let developers to take the language from context and apply any transformations to the errors they see fit.
From the API level we expose one more hook:
To apply the translation a developer might:
Accept-Language
header, validate it is a supported language and put the value into Context.To keep backward compatibility and to avoid introducing performance regressions the code is a bit boilerplaty. Please let me know what you think about such approach. I didn't fully cover it with tests yet, want to get your feedback first.
Below are the results of the benchmark on my machine (please keep in mind I had to disable few tests because they are failing on main branch):
Summary by CodeRabbit
New Features
Bug Fixes
Tests