Conversation
WalkthroughThe pull request refactors the codebase to use a unified context management system. Legacy parsing contexts and options have been replaced with execution contexts (ExecCtx, SchemaCtx, and Ctx) and ExecOptions. Schema methods across various types now accept a single context parameter rather than multiple individual parameters. Error handling has been restructured, with legacy methods removed and new error-reporting functions added. Tests and documentation have been updated accordingly, and new constants have been introduced for error key management. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as User/Client
participant Schema as Schema Processor
participant Ctx as ExecCtx/SchemaCtx
Client->>Schema: Call Parse/Validate(data, dest, ExecOption)
Schema->>Ctx: Create new execution context (NewExecCtx/NewSchemaCtx)
Schema->>Ctx: Invoke process(ctx) for pre-transform and validation
Ctx->>Schema: Report issues/errors via AddIssue
Schema-->>Client: Return error list/result from validation
Poem
✨ Finishing Touches
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Deploying zog with
|
| Latest commit: |
3e2c8ff
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://30ce56f3.zog-3a0.pages.dev |
| Branch Preview URL: | https://feat-context.zog-3a0.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (26)
utilsOptions.go (2)
39-41: Deprecation note is clear
DeprecatingParsingOptionin favor ofExecOptionis coherent with the new naming. Consider scheduling its removal once external references are migrated.
48-52: WithCtxValue approach
The method sets key-value pairs in the context. This is concise and effective. If overwriting existing keys should be handled differently, consider clarifying.boolean.go (1)
58-61: process method consistency
InvokingprimitiveProcessorwith the new context-based approach is clear. Consider adding doc comments if needed.internals/contexts.go (3)
5-16: Ctx interface design
The interface provides essential context operations and notes deprecation forNewError. It might be prudent to remove the deprecated signature in the future.
39-44: Set(key, val)
Initializes the map if necessary and stores data. This is intuitive. Clarify concurrency expectations if used in parallel contexts.
58-62: NewError (deprecated)
Mirrors AddIssue but is explicitly deprecated. Move usage to AddIssue and remove once adoption of AddIssue is complete.zogSchema.go (3)
49-49: Consider additional error context.While
ctx.IssueFromUnknownError(err)is effective, consider including more details (e.g., function name) to help debugging.
138-138: Unknown error handling.Consider capturing the context or transform source to aid debugging.
154-154: Unknown error in preTransforms.Ensure a fallback or partial rollback mechanism is in place if partial transforms have already occurred.
time.go (2)
49-50: Clarify usage limitation.The comment warns about only supporting
Schema.Parse. If there's a plan to supportValidateor other flows, update documentation accordingly.
62-62: Repeated warning.Reiterates the same limitation. Consider consolidating these warnings.
struct.go (3)
35-35: User-facing label is helpful.A concise header comment clarifying the function group is beneficial for maintainability.
107-110: Coercion error fallback.Your fallback logic is clear, though re-check the branches for possible edge cases with partial transforms.
159-159: Execution context initialization for validation.While consistent, ensure that specialized error formatters remain an option for advanced usage.
slices.go (1)
51-62: Clarify doc comment about “pointer pointer.”
The comment states "Validates a pointer pointer," but the function signature accepts a genericdata any. Consider refining the comment to reflect the actual parameter usage.string.go (2)
70-73: Consider reducing parameter proliferation.
primitiveProcessortakes numerous parameters. Refactoring them into a struct or grouping them could simplify function calls and enhance maintainability.
87-90: Similarly consider parameter grouping here.
primitiveValidatoralso requires many arguments. A more compact parameter signature might help.pointers.go (2)
53-73: Consider adding error handling for reflect operations.While the pointer handling logic is correct, the reflect operations could panic in edge cases.
Consider adding error handling:
func (v *PointerSchema) process(ctx *p.SchemaCtx) { isZero := p.IsParseZeroValue(ctx.Val, ctx) if isZero { if v.required != nil { ctx.AddIssue(ctx.IssueFromTest(v.required, ctx.Val)) } return } rv := reflect.ValueOf(ctx.DestPtr) + if !rv.IsValid() || rv.Kind() != reflect.Ptr { + ctx.AddIssue(ctx.IssueFromTest(&p.Test{ErrCode: zconst.ErrCodeFallback}, ctx.Val)) + return + } destPtr := rv.Elem() if destPtr.IsNil() { newVal := reflect.New(destPtr.Type().Elem()) destPtr.Set(newVal) } di := destPtr.Interface() ctx.DestPtr = di v.schema.process(ctx) }
86-98: Consider adding validation for the input value type.The validate method should verify that the input value is a pointer before proceeding with validation.
Consider adding type validation:
func (v *PointerSchema) validate(ctx *p.SchemaCtx) { rv := reflect.ValueOf(ctx.Val) + if !rv.IsValid() || rv.Kind() != reflect.Ptr { + ctx.AddIssue(ctx.IssueFromTest(&p.Test{ErrCode: zconst.ErrCodeFallback}, ctx.Val)) + return + } destPtr := rv.Elem() if !destPtr.IsValid() || destPtr.IsNil() { if v.required != nil { ctx.AddIssue(ctx.IssueFromTest(v.required, ctx.Val)) } return } di := destPtr.Interface() ctx.Val = di v.schema.validate(ctx) }i18n/i18n_test.go (2)
79-85: Consider using table-driven test data for structures.The test could be more maintainable by moving the test structures into the test cases.
Consider refactoring to:
type testCase struct { name string lang string input map[string]interface{} + dest interface{} + dest2 interface{} expectedErr bool expected string }
87-101: Consider consolidating duplicate assertions.The validation assertions for both structures are nearly identical and could be consolidated to reduce code duplication.
Consider creating a helper function:
func assertValidationErrors(t *testing.T, errs p.ZogErrMap, expectedMsg string) { nameErrs, ok := errs["name"] assert.True(t, ok, "Expected errors for 'name' field") assert.NotEmpty(t, nameErrs, "Expected at least one error for 'name' field") assert.Equal(t, expectedMsg, nameErrs[0].Message(), "Unexpected error message") }utils.go (1)
10-12: Verify deprecation timeline.The deprecation notice should include information about when the type will be removed and what users should migrate to.
Consider adding more details to the deprecation notice:
-// Deprecated: Use z.Ctx instead. -// ParseCtx will be removed in the future since it is used for both validation and parsing and is a confusing name. +// Deprecated: Use z.Ctx instead. ParseCtx will be removed in v1.0.0 since it is used for both validation and parsing +// and is a confusing name. Migrate your code to use z.Ctx for all context-related operations.numbers.go (1)
74-78: Consider adding error handling documentation.While the process method is correctly implemented, it would benefit from documentation about error handling behavior.
Add documentation about error handling:
+// Internal function to process the data. +// Errors are collected in the context's error list and can be accessed after processing. +// Zero values are detected using IsParseZeroValue. func (v *NumberSchema[T]) process(ctx *p.SchemaCtx) { primitiveProcessor(ctx, v.preTransforms, v.tests, v.postTransforms, v.defaultVal, v.required, v.catch, v.coercer, p.IsParseZeroValue) }numbers_validate_test.go (1)
10-18: Consider adding edge cases to the test.The test only covers a simple case. Consider adding test cases for:
- Zero values
- Negative numbers
- Maximum/minimum integer values
numbers_test.go (1)
10-18: Consider adding edge cases to the parsing test.The test only covers a simple case. Consider adding test cases for:
- String to number conversion
- Invalid input types
- Overflow/underflow cases
struct_helper_test.go (1)
315-349: Consider adding more test cases for the Extend functionality.The current test coverage is good, but could be enhanced with additional cases:
- Test extending with multiple fields
- Test extending with transforms
- Test extending an already extended schema
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
boolean.go(1 hunks)boolean_test.go(3 hunks)boolean_validate_test.go(1 hunks)conf/Errors.go(1 hunks)docs/docs/errors.md(1 hunks)i18n/i18n.go(1 hunks)i18n/i18n_test.go(2 hunks)internals/contexts.go(1 hunks)internals/errors.go(8 hunks)internals/parsing.go(0 hunks)internals/tests.go(10 hunks)internals/types.go(1 hunks)internals/zeroValues.go(2 hunks)numbers.go(1 hunks)numbers_test.go(1 hunks)numbers_validate_test.go(1 hunks)pointers.go(2 hunks)pointers_test.go(3 hunks)pointers_validate_test.go(3 hunks)slices.go(2 hunks)string.go(2 hunks)struct.go(5 hunks)struct_helper_test.go(5 hunks)time.go(3 hunks)utils.go(4 hunks)utilsOptions.go(1 hunks)utilsOptions_test.go(1 hunks)zconst/consts.go(1 hunks)zogSchema.go(9 hunks)
💤 Files with no reviewable changes (1)
- internals/parsing.go
✅ Files skipped from review due to trivial changes (1)
- docs/docs/errors.md
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (130)
utilsOptions.go (2)
37-37: ExecOption introduced successfully
This new type alias cleanly transitions from parsing to execution context, aligning with the broader refactor.
42-46: WithErrFormatter is straightforward
The function correctly wraps the formatter logic in anExecOption, promoting better flexibility for error formatting.boolean.go (7)
47-47: Parse method signature updated correctly
Switching fromParsingOptiontoExecOptionaligns with the new context model.
49-49: Default error formatter
Usingconf.ErrorFormatteras a default is logical. Future overrides via options are supported.
53-53: Schema context usage
Passing the new schema context intov.processis consistent with the overall refactor.
64-64: Validate now uses ExecOption
Signature changes match the parse flow, ensuring uniform context usage in both parse and validate paths.
66-66: Creation of a new ExecCtx
Same pattern as in Parse. Good consistency for error collection and handling.
71-71: validate call with schema context
Usage ofctx.NewSchemaCtxfor validation aligns with the overall design.
76-77: validate method
Delegating toprimitiveValidatorthrough the new context simplifies the validation logic.internals/contexts.go (17)
1-4: Package and import statements
Defininginternalsas the package and importing zconst is fine. No issues spotted.
18-23: NewExecCtx factory
Creates a well-scoped execution context with a custom error formatter. Straightforward.
25-29: ExecCtx struct
Holds formatter, error handling, and a map for ephemeral data. For multi-goroutine usage, lock management may eventually be required.
31-33: HasErrored
Simple check against stored errors. Looks good.
35-37: SetErrFormatter
Allows dynamic error formatter updates. Straightforward.
46-48: Get(key string) any
Retrieves map values with nil fallback. Behavior is typical and fine.
50-56: AddIssue
Defers to the provided error formatter if message is empty, then accumulates the error. Solid approach for flexible error handling.
64-70: FmtErr
Handles partial formatting logic if no message is set. Straightforward, well-contained.
72-80: NewSchemaCtx
Generates a specializedSchemaCtx. Clear separation of concerns and data flow.
82-89: NewValidateSchemaCtx
Similar to NewSchemaCtx but specialized for validation flows. Consistent naming.
91-101: SchemaCtx struct
Embeds ExecCtx for shared error handling plus schema-specific fields. Good composition pattern.
103-110: Issue generation
Builds a baseZogErrwith relevant data. Straightforward.
112-125: IssueFromTest
Generates a specialized error from a test, injecting error code and parameters. Clear delineation of responsibilities.
127-136: IssueFromCoerce
Captures coercion errors in a standardizedZogErr. Straightforward.
138-146: IssueFromUnknownError
Wraps unrecognized errors inZogErr, facilitating consistent internal error representation.
148-156: TestCtx struct
ExtendsSchemaCtxwith test-specific logic. This preserves context across tests well.
159-175: TestCtx FmtErr / AddIssue
Leverages either custom test-level error formatting if provided or falls back to the underlyingSchemaCtxlogic. Good layering.zogSchema.go (17)
12-13: Well-structured method signatures for context-based processing.The migration to a single
*p.SchemaCtxparameter helps unify and simplify the method calls. This change follows a consistent pattern across the codebase, favoring a more cohesive approach to context handling.
22-22: Updated Parse method signature with ExecOption.Adopting
ExecOptionensures consistency with other schema methods and aligns with the streamlined context-driven approach.
29-29: Consistent Parse method signature for primitives.Switching from
ParsingOptiontoExecOptionunifies the schema's option handling. Good job maintaining coherence.
34-34: Primitive processor refactor.This signature neatly encapsulates all necessary parameters in the context. It’s straightforward and aids future extensibility.
38-38: Validate cast safety for ctx.DestPtr.Ensure
ctx.DestPtris never nil or an incompatible type, to avoid runtime panics.Would you like a script to scan for all usage points of
ctx.DestPtrto confirm correct types?
41-41: Pre-transform invocation logic.Looks good, but ensure each pre-transform function robustly handles unexpected input to avoid partial data corruption.
52-52: Assigning transform result to ctx.Val.This is consistent with the new approach. Keep an eye on data type assumptions downstream.
73-73: Zero-value check with custom function.Preserving the flexible
isZeroFuncapproach is good. No issues spotted.
88-88: Adding issue on required test failure.Appropriate usage of
ctx.AddIssue. The error is recorded gracefully.
93-93: Coercion step.This neatly centralizes type conversion logic. Make sure coercers handle all edge cases (e.g., nil input).
101-101: Coerce error handling.Use of
IssueFromCoerceis clear. Good call for future categorization of coercion-related issues.
119-119: Flagging test failures with IssueFromTest.Enforces test-based validations consistently with the new context-based approach.
126-126: Primitive validator refactor.Mirroring the processor changes ensures uniform handling for parse and validate.
130-130: Pointer cast in validator.Confirm the pointer and cast correctness for each schema usage to prevent type assertion panics.
136-136: Post-transform invocation in validator.Ensures transformations happen in the same way as parsing. Nice approach.
177-177: Notify on test failure for required fields.Consistent approach to required checks. Good usage of
IssueFromTest.
190-190: Appending test failures for additional validations.Matches the rest of the approach in capturing relevant data for debugging.
time.go (7)
73-73: Switched Parse to ExecOption.Ensures alignment with the new context-based approach.
75-75: Initialize execution context with new error formatter.Implementation looks consistent with the pattern used across other schemas.
81-81: Invoking process with newly created schema context.Keeps the code DRY and consistent. Nicely follows the established pattern.
86-89: Internal process method refactor.Delegating to
primitiveProcessorcentralizes logic. This is a solid improvement for maintainability.
91-97: New Validate method for time.Time.This addition nicely complements
Parse. Consistent usage of the same context-based approach with relevant options.
98-100: Validate invocation and return.Straightforward flow. Ensure all error edge cases are properly tested.
102-105: Internal validation for TimeSchema.Mirroring
processviaprimitiveValidatorensures consistent handling for parse vs. validate.struct.go (27)
37-38: Defining Schema map & doc comment.This is clear and fosters strong typing. Great for referencing fields in a structured manner.
40-41: Struct function returning a StructSchema.Helps keep the code easy to consume. The default configuration with a provided schema is straightforward.
48-48: New Parse method.Shifting to a context-based approach keeps usage consistent across multiple schema implementations.
49-50: Error map & ExecCtx creation.Ensures granular error recording. Good job exposing a structured error response.
51-52: Applying ExecOptions.Allows customization of the context. This pattern shows flexibility and uniform usage.
55-57: Process invocation and returning error map.Wrapping the call with a new
SchemaCtxmakes the solution clean.
60-60: StructSchema process method.Provides clarity on how the transformations, validations, and final assignment occur.
64-64: PreTransform function invocation.Each transform properly receives the current value and context.
67-67: Attaching errors on preTransform failures.Raising issues with
ctx.AddIssueensures consistent aggregator usage.
70-70: Updating ctx.Val after transforms.Clear flow: input → transform → updated ctx.Val.
79-79: Applying postTransforms on destPtr.Ensures changes are made directly to the final destination.
81-81: Appending error with SetError.A consistent technique to unify error representation.
89-89: Checking for implicit EmptyDataProvider.This is a neat approach to handle optional or missing data.
92-92: Record missing required struct.Correctly flags an error if
isEmptybut the field is mandatory.
97-97: Data provider creation.Ties in well with your approach for complex data structures.
113-114: Handle required + coerce error.Appropriate use of
IssueFromTestfor structured consistency.
119-119: Reflecting destination pointer.Use of
Elem()is standard. Ensure no nil pointer is accidentally passed.
139-141: Sub-schema processing.Splitting nested providers for nested schemas is a clear, logical approach. Good job.
148-149: Struct-level tests.Validates the final constructed struct pointer thoroughly. Nicely done.
155-155: New Validate method.Mirrors
Parsebut focuses on existing structs. Maintains the same error map approach.
164-164: Internal validate invocation.Reusing common patterns consolidates code paths for parse vs. validate.
170-183: Deferred postTransforms in validation.This parallels the parse flow, ensuring transformations only on successful validation. The logic flow is coherent.
184-184: Retrieving reflected value by pointer.Check for edge cases if
dataPtrisnilor non-struct.
186-196: Invoking preTransforms in Validate.Similar to parse, each transform gets the current struct value. The fallback design is consistent.
198-210: Required struct handling.Handles default vs. required logic well. If the struct is zero-value but required, an error is recorded.
212-228: Validating each field with sub-schema.Building the field key and leveraging reflection is logical. Very flexible for diverse struct definitions.
230-236: Struct-level tests and final transformations.This block ensures you test the final struct and only run postTransforms if there are no errors.
slices.go (18)
12-12: No changes needed.
36-36: No changes needed.
38-49: Constructor looks good!
The functional options pattern is clearly implemented, and the default coercer assignment is convenient.
64-65: Internal validate method looks fine.
82-91: Pre-transform implementation is consistent.
94-105: Default/required logic is clear.
110-114: Per-item validation appears correct.
118-125: Top-level slice test stage is well-handled.
131-138: Parse method implementation is appropriate.
144-156: Process method structure is consistent.
175-175: Zero-value check call is correct.
178-188: Default/required handling for parse path is well-structured.
190-195: Coercer usage and error handling appear valid.
198-199: Slice resizing matches the source slice length.
203-208: Per-item parse logic is correct.
213-214: Slice-level tests after parse are well-implemented.
71-80: Verify post-transform data pointer usage.
In this validation path, the post-transforms callfn(ctx.Val, ctx). Ensurectx.Valis always a pointer to avoid potential runtime panics.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for all references of `ctx.Val` usage in the codebase # to confirm that it is consistently assigned as a pointer before post-transforms. rg -A 5 "ctx.Val"Length of output: 6832
Pointer Usage Verified in Post-Transforms
The search output confirms that before being passed to functions (e.g., during post-transforms where
fn(ctx.Val, ctx)is invoked),ctx.Valis consistently treated as a pointer across the codebase. Notable confirmations include:
- Dereferencing Check: Multiple files (e.g.,
slices.go,struct.go,pointers.go) invokereflect.ValueOf(ctx.Val).Elem(), ensuring thatctx.Valis a pointer before any dereference is performed.- Type Assertions: In
zogSchema.go,ctx.Valis type-asserted to a pointer (e.g.,ctx.Val.(*T)), which would immediately fail if it were not appropriately a pointer.- Early Error Handling: Cases in
pointers.goinvolve explicit validity and nil checks onctx.Val, supporting a robust safety net against runtime panics.Based on these points, the handling of
ctx.Valas a pointer in post-transform paths appears consistent and safe.
164-164: Verify post-transform consistency with DestPtr.
Here,fn(ctx.DestPtr, ctx)is used instead ofctx.Val. Confirm that this distinction is intentional and consistent in the codebase.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Compare usage of post-transforms in parse vs. validate paths. rg -A 3 "fn(ctx.(Val|DestPtr), ctx)"Length of output: 38
I'm seeing no output from the previous command—likely because the regex wasn't escaping the delimiters. Let's re-run our searches with properly escaped patterns to check for consistency between calls using
ctx.DestPtrversusctx.Valin the file.Please run this script to verify the usage.
🏁 Script executed:
#!/bin/bash echo "Searching for occurrences of fn(ctx.DestPtr, ctx) in slices.go:" rg -n "fn\(ctx\.DestPtr, ctx\)" slices.go echo "" echo "Searching for occurrences of fn(ctx.Val, ctx) in slices.go:" rg -n "fn\(ctx\.Val, ctx\)" slices.goLength of output: 464
Consistency Verified: The Use of
ctx.DestPtrIs IntentionalThe search results confirm that while earlier parts of the code (lines 71 and 149) use
fn(ctx.Val, ctx), the post-transform phase at line 164 correctly switches tofn(ctx.DestPtr, ctx). This distinction is consistent with the intended design of applying post-transform modifications. No further changes are needed.string.go (3)
56-56: Parsing method signatures look solid.
75-85: Validate method is correct.
The execution context creation and schema validation flow are appropriately handled.
103-103: Updated signature reflects the new context usage.internals/types.go (2)
6-6: Switching from ParseCtx to Ctx aligns with the new architecture.
9-9: Likewise, the PostTransform signature is consistent with the recent refactor.internals/zeroValues.go (2)
8-8: LGTM! Type alias updated to use unified context.The change from
ParseCtxtoCtxaligns with the PR objective of unifying context management.
27-27: LGTM! Function signature updated to use unified context.The change from
ParseCtxtoCtxaligns with the PR objective of unifying context management.utilsOptions_test.go (4)
11-11: LGTM! Context creation updated to use execution context.The change from
NewParseCtxtoNewExecCtxaligns with the PR objective of unifying context management.
17-17: LGTM! Context creation updated to use execution context.The change from
NewParseCtxtoNewExecCtxaligns with the PR objective of unifying context management.
22-25: LGTM! Error handling updated to use new approach.The changes align with the PR objective:
- Explicit initialization of
EPathfield.- Using
AddIssueinstead ofNewErrorfor error reporting.
31-31: LGTM! Function parameter type updated to use unified context.The change from
ParseCtxtoCtxaligns with the PR objective of unifying context management.i18n/i18n.go (1)
26-26: LGTM! Error formatter updated to use unified context.The change from
internals.ParseCtxtointernals.Ctxaligns with the PR objective of unifying context management.conf/Errors.go (1)
19-19: LGTM! Error formatter updated to use unified context.The change from
p.ParseCtxtop.Ctxaligns with the PR objective of unifying context management.pointers.go (2)
40-51: LGTM! Clean transition to the new context management system.The Parse method has been successfully updated to use ExecOption and the new context creation pattern, which aligns with the broader refactoring goals.
76-84: LGTM! Clean implementation of the Validate method.The Validate method correctly implements the new context management pattern.
zconst/consts.go (1)
4-24: LGTM! Well-documented error key constants.The new error key constants are clearly documented with examples, which will help developers understand their usage.
pointers_validate_test.go (2)
26-37: LGTM! Good test coverage for custom error formatting.The test effectively validates both custom and default error formatting scenarios.
138-138: LGTM! Proper use of the new error key constant.The update to use
zconst.ERROR_KEY_ROOTaligns with the new error handling approach.internals/tests.go (2)
10-11: LGTM! Well-documented type alias.The
TestFunctype alias provides clear type safety and documentation for validation functions.
13-19: LGTM! Well-structured test representation.The
Teststruct provides a clean and organized way to represent validation tests with all necessary components.utils.go (1)
21-21: LGTM! Clear type alias.The
ZogIssuetype alias provides a more descriptive name for error handling.pointers_test.go (2)
32-43: LGTM! Comprehensive error formatting test.The test thoroughly verifies both custom and default error message formatting for pointer validation.
45-54: LGTM! Good coercer test coverage.The test effectively verifies that the coercer function is properly passed through the pointer validator.
numbers.go (1)
62-72: LGTM! Clean context management.The Parse method now uses the new execution context consistently with other schema types.
internals/errors.go (5)
13-64: LGTM! Well-structured interface changes.The new methods follow a consistent naming pattern and enable method chaining. The deprecated methods are properly marked with comments.
72-72: LGTM! Context type updated.The change from
ParseCtxtoCtxaligns with the PR's context management refactor.
77-77: LGTM! Error path field added.The
EPathfield is properly typed and documented.
89-155: LGTM! Method implementations are consistent.The implementations follow a consistent pattern and properly enable method chaining.
211-211: LGTM! Error keys centralized.Using error key constants from the
zconstpackage improves maintainability.Also applies to: 216-216
numbers_validate_test.go (1)
20-31: LGTM! Well-structured error formatting test.The test effectively validates both custom and default error messages.
numbers_test.go (1)
20-31: LGTM! Well-structured parsing error formatting test.The test effectively validates both custom and default error messages during parsing.
boolean_validate_test.go (1)
38-46: LGTM! Context type updated consistently.The function name and parameter type changes align with the PR's context management refactor.
struct_helper_test.go (1)
56-57: LGTM! Context type updates align with the unified context management system.The changes from
ParseCtxtoCtxin transform functions are consistent with the PR's objective of standardizing context management.Also applies to: 63-64, 85-86, 92-93, 202-203, 232-233
boolean_test.go (2)
66-74: LGTM! Function rename and context type update.The changes from
TestParsingOptiontoTestExecOptionand*p.ZogParseCtxto*p.ExecCtxalign well with the unified context management system.
115-115: LGTM! Good error message validation.The addition of error message testing improves the test coverage by ensuring that custom error messages are properly propagated.
Also applies to: 126-128
| fieldMeta, ok := structVal.Type().FieldByName(key) | ||
| if !ok { | ||
| panic(fmt.Sprintf("Struct is missing expected schema key: %s", key)) | ||
| panic(fmt.Sprintf("Struct is missing expected schema key: %s\n see the zog FAQ for more info", key)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Panic on missing struct field.
While explicit, consider safer error handling to avoid runtime panics for user-supplied schemas.
- panic(fmt.Sprintf("Struct is missing expected schema key: %s\n see the zog FAQ for more info", key))
+ ctx.AddIssue(
+ ctx.Issue().SetMessage(
+ fmt.Sprintf("Struct is missing expected schema key: %s", key),
+ ),
+ )
+ return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| panic(fmt.Sprintf("Struct is missing expected schema key: %s\n see the zog FAQ for more info", key)) | |
| ctx.AddIssue( | |
| ctx.Issue().SetMessage( | |
| fmt.Sprintf("Struct is missing expected schema key: %s", key), | |
| ), | |
| ) | |
| return |
| ValidateFunc: func(val any, ctx Ctx) bool { | ||
| x := val.(T) | ||
| return len(x) >= n | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify error handling for type assertion.
The type assertion val.(T) in LenMin could panic if the value is not of type T. Consider adding a type check similar to the one in LenMax.
Apply this diff to add type checking:
- x := val.(T)
- return len(x) >= n
+ x, ok := val.(T)
+ if !ok {
+ return false
+ }
+ return len(x) >= n📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ValidateFunc: func(val any, ctx Ctx) bool { | |
| x := val.(T) | |
| return len(x) >= n | |
| }, | |
| ValidateFunc: func(val any, ctx Ctx) bool { | |
| x, ok := val.(T) | |
| if !ok { | |
| return false | |
| } | |
| return len(x) >= n | |
| }, |
1 participant
Summary by CodeRabbit
New Features
Refactor
Documentation