perf: internal tests for primitive values now use pointers. User tests are unaffected#97
perf: internal tests for primitive values now use pointers. User tests are unaffected#97
Conversation
… tests will still get a copy of the value.
WalkthroughThe pull request updates various schema validations by introducing a new backwards compatibility mechanism. In several schema types (BoolSchema, NumberSchema, TimeSchema), the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant S as Schema.Test Method
participant W as customTestBackwardsCompatWrapper
C->>S: Invoke Test(t)
S->>W: Wrap t.ValidateFunc
W-->>S: Return wrapped function
S->>C: Return updated schema with wrapped test
Possibly related PRs
Poem
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
utils.go (1)
204-209: Consider optimizing reflection usage for better performance.While the wrapper successfully maintains backwards compatibility by dereferencing pointer values, the use of reflection could impact performance. Consider:
- Caching the reflect.Value to avoid repeated reflection calls.
- Using type switches for common primitive types to avoid reflection overhead.
Example optimization:
func customTestBackwardsCompatWrapper(testFunc p.TestFunc) func(val any, ctx Ctx) bool { return func(val any, ctx Ctx) bool { - refVal := reflect.ValueOf(val).Elem() - return testFunc(refVal.Interface(), ctx) + switch v := val.(type) { + case *string: + return testFunc(*v, ctx) + case *int: + return testFunc(*v, ctx) + case *float64: + return testFunc(*v, ctx) + case *bool: + return testFunc(*v, ctx) + case *time.Time: + return testFunc(*v, ctx) + default: + refVal := reflect.ValueOf(val).Elem() + return testFunc(refVal.Interface(), ctx) + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
boolean.go(1 hunks)error_msgs_test.go(1 hunks)internals/tests.go(9 hunks)numbers.go(1 hunks)string.go(11 hunks)time.go(4 hunks)utils.go(2 hunks)zogSchema.go(2 hunks)
🔇 Additional comments (19)
boolean.go (1)
90-90: LGTM! Good use of backward compatibility wrapper.The addition of
customTestBackwardsCompatWrapperaligns with the PR's objective of using pointers for internal tests while maintaining backward compatibility for user tests.internals/tests.go (1)
41-42: LGTM! Consistent pointer-based type assertions across all validation functions.The changes maintain type safety while improving performance by:
- Using
.(*T)for pointer-based type assertions- Dereferencing values with
*vfor comparisons- Preserving error handling for failed type assertions
Also applies to: 54-58, 70-74, 87-88, 104-108, 120-124, 136-140, 152-156, 168-172
zogSchema.go (1)
113-113: LGTM! Consistent pointer handling in validation functions.The changes correctly update the validation function calls to pass pointers directly instead of dereferencing them, which:
- Aligns with the PR's performance optimization goal
- Maintains consistency with the pointer-based approach in other files
- Preserves existing error handling mechanisms
Also applies to: 185-185
numbers.go (1)
153-153: LGTM! Backwards compatibility wrapper added.The addition of
customTestBackwardsCompatWrapperaligns with the PR objectives to enhance performance by using pointers while maintaining backwards compatibility.time.go (4)
166-166: LGTM! Backwards compatibility wrapper added.The addition of
customTestBackwardsCompatWrapperaligns with the PR objectives to enhance performance by using pointers while maintaining backwards compatibility.
186-186: LGTM! Type assertion updated to use pointer.The change from
time.Timeto*time.Timetype assertion aligns with the PR objectives to enhance performance by using pointers.
208-208: LGTM! Type assertion updated to use pointer.The change from
time.Timeto*time.Timetype assertion aligns with the PR objectives to enhance performance by using pointers.
230-230: LGTM! Type assertion updated to use pointer.The change from
time.Timeto*time.Timetype assertion aligns with the PR objectives to enhance performance by using pointers.string.go (11)
165-165: LGTM! Backwards compatibility wrapper added.The addition of
customTestBackwardsCompatWrapperaligns with the PR objectives to enhance performance by using pointers while maintaining backwards compatibility.
222-222: LGTM! Type assertion updated to use pointer.The change from
stringto*stringtype assertion aligns with the PR objectives to enhance performance by using pointers.Also applies to: 226-226
241-241: LGTM! Type assertion updated to use pointer.The change from
stringto*stringtype assertion aligns with the PR objectives to enhance performance by using pointers.Also applies to: 245-245
262-262: LGTM! Type assertion updated to use pointer.The change from
stringto*stringtype assertion aligns with the PR objectives to enhance performance by using pointers.Also applies to: 266-266
283-283: LGTM! Type assertion updated to use pointer.The change from
stringto*stringtype assertion aligns with the PR objectives to enhance performance by using pointers.Also applies to: 287-287
304-304: LGTM! Type assertion updated to use pointer.The change from
stringto*stringtype assertion aligns with the PR objectives to enhance performance by using pointers.Also applies to: 308-308
324-324: LGTM! Type assertion updated to use pointer.The change from
stringto*stringtype assertion aligns with the PR objectives to enhance performance by using pointers.Also applies to: 328-328
348-348: LGTM! Type assertion updated to use pointer.The change from
stringto*stringtype assertion aligns with the PR objectives to enhance performance by using pointers.Also applies to: 352-352
375-375: LGTM! Type assertion updated to use pointer.The change from
stringto*stringtype assertion aligns with the PR objectives to enhance performance by using pointers.Also applies to: 379-379
402-402: LGTM! Type assertion updated to use pointer.The change from
stringto*stringtype assertion aligns with the PR objectives to enhance performance by using pointers.Also applies to: 406-406
422-422: LGTM! Type assertion updated to use pointer.The change from
stringto*stringtype assertion aligns with the PR objectives to enhance performance by using pointers.Also applies to: 426-426
|
|
||
| errs := schema.Parse(map[string]any{"name": "0", "age": 0, "time": "2020-01-01T00:00:00Z", "bool": false, "slice": []string{"x"}}, &u) | ||
| sanitized := Errors.SanitizeMap(errs) | ||
| sanitized := Issues.SanitizeMapAndCollect(errs) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the migration from Errors to Issues package.
The change from Errors.SanitizeMap to Issues.SanitizeMapAndCollect suggests a package migration. Let's verify that all usages have been updated consistently.
🏁 Script executed:
#!/bin/bash
# Search for any remaining usages of the old Errors.SanitizeMap
rg "Errors\.SanitizeMap"
# Search for all usages of the new Issues.SanitizeMapAndCollect to ensure consistent adoption
rg "Issues\.SanitizeMapAndCollect"Length of output: 600
Migration Verification: Inconsistent Package References
- In
error_msgs_test.go(line 30), the call toIssues.SanitizeMapAndCollect(errs)is correctly updated. - However, residual references to
Errors.SanitizeMapremain in the documentation (e.g., indocs/docs/examples-of-use/rest-apis.mdanddocs/docs/packages/zenv.md). Please verify if these need to be updated for consistent migration across the project.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internals/contexts.go (1)
147-148: Consider removing redundant error initialization.The
Errfield is initialized tonilon line 147 but is immediately overwritten with the provided error on line 151. Consider removing the redundant initialization.func (c *SchemaCtx) IssueFromCoerce(err error) ZogIssue { e := ZogIssuePool.Get().(*ZogErr) e.C = zconst.IssueCodeCoerce e.EPath = c.Path.String() - e.Err = nil e.Msg = "" e.Typ = c.DType e.Val = c.Val e.Err = err return e }Also applies to: 151-151
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internals/contexts.go(3 hunks)
🔇 Additional comments (2)
internals/contexts.go (2)
113-117: LGTM! Consistent error field initialization.The explicit initialization of error fields ensures consistent error handling and prevents potential nil pointer dereferences.
128-129: LGTM! Consistent error initialization in test context.The initialization maintains uniformity with the error handling pattern established in
Issue().
Deploying zog with
|
| Latest commit: |
377ab8c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://21a59d08.zog-3a0.pages.dev |
| Branch Preview URL: | https://perf-test-ptrs.zog-3a0.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/docs/examples-of-use/rest-apis.md (1)
7-7: Typo Correction Suggestion:
Consider changing "Zog providers" to "Zog provides" on line 7 for correct grammar and clarity.docs/docs/packages/zenv.md (1)
47-47: Formatting Issue – Replace Hard Tabs:
Static analysis reported hard tabs on this line. Please replace hard tabs with spaces to adhere to the markdown lint guidelines.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
47-47: Hard tabs
Column: 1(MD010, no-hard-tabs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/docs/examples-of-use/rest-apis.md(1 hunks)docs/docs/packages/zenv.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/docs/packages/zenv.md
47-47: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
docs/docs/examples-of-use/rest-apis.md (1)
13-13: Consistent Usage of Issues Namespace:
The example now correctly callsz.Issues.SanitizeMap(errs)instead of the deprecatedz.Errors.SanitizeMap(errsMap), ensuring consistency with the new error handling approach.
1 participant
… tests will still get a copy of the value.
Summary by CodeRabbit