feat: add not operator #76
Conversation
|
Warning Rate limit exceeded@Jh123x has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe changes introduce support for a NOT operator for test validations. A new function, Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Function Caller
participant Utils as AddTest function
participant TestObj as Test object
Caller->>Utils: Call AddTest(testArr, t, isNot)
alt isNot is false
Utils->>TestObj: Append test as is
else isNot is true
Utils->>TestObj: Wrap ValidateFunc in negation
Utils->>TestObj: Append modified test
end
Utils->>Caller: Return updated testArr
sequenceDiagram
participant User as Schema User
participant Schema as StringSchema
participant Utils as AddTest function
User->>Schema: Call Not() to toggle negation
User->>Schema: Invoke validation method (e.g., Test, Min, etc.)
Schema->>Schema: Call addTest() using current isNot flag
Schema->>Utils: Delegate test addition to AddTest(...)
Schema->>User: Return validation result / updated schema
Possibly related PRs
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 0
🧹 Nitpick comments (1)
string_test.go (1)
382-416: Consider adding more test cases for comprehensive coverage.While the current test cases validate basic functionality, consider adding cases for:
- Multiple chained Not() calls
- Not() with other validators (Email, URL, etc.)
- Edge cases (empty string, special characters)
Here's an example of additional test cases:
tests := map[string]struct { schema *StringSchema strVal string expectedErrMap internals.ZogErrList }{ // ... existing test cases ... + "double not": { + schema: String().Not().Not().Len(4), + strVal: "test", + expectedErrMap: nil, + }, + "not email": { + schema: String().Not().Email(), + strVal: "not-an-email", + expectedErrMap: nil, + }, + "not with empty": { + schema: String().Not().Len(0), + strVal: "", + expectedErrMap: internals.ZogErrList{ + &internals.ZogErr{ + C: "len", + ParamsM: map[string]any{"len": 0}, + Typ: "string", + Val: "", + Msg: "string must be exactly 0 character(s)", + Err: nil, + }, + }, + }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internals/utils.go(1 hunks)string.go(17 hunks)string_test.go(2 hunks)
🔇 Additional comments (4)
internals/utils.go (1)
21-32: LGTM! Clean implementation of the NOT operator.The function elegantly handles test negation by wrapping the original validation function when
isNotis true, while maintaining the original behavior whenisNotis false.string.go (3)
29-29: LGTM! Well-placed field addition.The
isNotfield is appropriately placed in theStringSchemastruct.
412-415: LGTM! Clean implementation of the Not() method.The method follows the builder pattern and toggles the
isNotflag correctly.
417-424: LGTM! Well-structured centralization of test addition logic.The
addTestmethod effectively:
- Uses the utility function for test addition
- Handles the
isNotflag reset- Maintains method chaining
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
i18n/en/en.go (1)
15-15: Fix grammatical issues in error messages.Some negated error messages have grammatical issues:
- Line 15: "string not must contain" should be "string must not contain"
- Line 17: "string must not contain at most" is confusing, consider "string must contain more than"
- Lines 37-39: "must contain any" should be "must not contain any"
Apply this diff to fix the grammar:
- internals.NotErrCode(zconst.ErrCodeMin): "string not must contain at least {{min}} character(s)", + internals.NotErrCode(zconst.ErrCodeMin): "string must not contain at least {{min}} character(s)", - internals.NotErrCode(zconst.ErrCodeMax): "string must not contain at most {{max}} character(s)", + internals.NotErrCode(zconst.ErrCodeMax): "string must contain more than {{max}} character(s)", - internals.NotErrCode(zconst.ErrCodeContainsUpper): "string must contain any uppercase letters", + internals.NotErrCode(zconst.ErrCodeContainsUpper): "string must not contain any uppercase letters", - internals.NotErrCode(zconst.ErrCodeContainsLower): "string must contain any lowercase letters", + internals.NotErrCode(zconst.ErrCodeContainsLower): "string must not contain any lowercase letters",Also applies to: 17-17, 37-37, 38-38, 39-39
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
i18n/en/en.go(1 hunks)internals/utils.go(2 hunks)string_test.go(2 hunks)
🔇 Additional comments (3)
internals/utils.go (2)
24-36: LGTM! Well-structured implementation of the NOT operator.The function elegantly handles both positive and negative cases, properly wraps the original validation function when negating, and appropriately modifies the error code.
38-43: LGTM! Clean implementation of error code transformation.The function effectively handles both adding and removing the "not_" prefix, making it versatile for both positive and negative cases.
string_test.go (1)
382-469: LGTM! Comprehensive test coverage for the NOT operator.The test cases effectively cover:
- Basic NOT operation
- Double negation
- Edge cases
- Various validation rules with NOT
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
i18n/en/en.go (1)
15-15: Improve clarity and consistency of error messages.Some error messages could be clearer:
- Line 15: "must not contain more {{min}}" is ambiguous
- Line 17: "must not contain more than {{max}}" has redundant wording
- Lines 24-25: Generic messages for Match validation could be more specific
Consider these improvements:
- internals.NotErrCode(zconst.ErrCodeMin): "string must not contain more {{min}} character(s)", + internals.NotErrCode(zconst.ErrCodeMin): "string must contain less than {{min}} character(s)", - internals.NotErrCode(zconst.ErrCodeMax): "string must not contain more than {{max}} character(s)", + internals.NotErrCode(zconst.ErrCodeMax): "string must contain more than {{max}} character(s)", - zconst.ErrCodeMatch: "string is invalid", - internals.NotErrCode(zconst.ErrCodeMatch): "string is valid", + zconst.ErrCodeMatch: "string must match the required pattern", + internals.NotErrCode(zconst.ErrCodeMatch): "string must not match the pattern",Also applies to: 17-17, 24-25
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
i18n/en/en.go(1 hunks)
🔇 Additional comments (3)
i18n/en/en.go (3)
4-4: LGTM!The import of the
internalspackage is necessary for using theNotErrCodefunction to generate negated error codes.
10-43: LGTM! Well-structured error messages for both positive and negative validations.The implementation follows a consistent pattern of pairing each validation rule with its negated counterpart, making the error messages comprehensive and clear.
46-90: Verify the scope of NOT operator support.Other validation types (bool, number, time, slice, struct) don't have negated messages yet. Is this intentional, or will they be added in future PRs?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
i18n/en/en.go (2)
14-17: Improve clarity of min/max error messages.The min/max error messages could be clearer by using "fewer" instead of "less" and being more explicit about the boundaries.
Apply this diff to improve message clarity:
- internals.NotErrCode(zconst.ErrCodeMin): "string must contain less than {{min}} character(s)", + internals.NotErrCode(zconst.ErrCodeMin): "string must contain fewer than {{min}} character(s)", - zconst.ErrCodeMax: "string must contain at most {{max}} character(s)", + zconst.ErrCodeMax: "string must not exceed {{max}} character(s)", - internals.NotErrCode(zconst.ErrCodeMax): "string must contain more than {{max}} character(s)", + internals.NotErrCode(zconst.ErrCodeMax): "string must exceed {{max}} character(s)",
46-90: Consider extending negation support to other types.The "not" operator support is currently implemented only for string validation rules. For consistency and completeness, consider extending this support to other types (bool, number, time, slice, struct).
Would you like me to help generate the negated error messages for other types?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
i18n/en/en.go(1 hunks)
🔇 Additional comments (1)
i18n/en/en.go (1)
4-4: LGTM!The import statement is correctly added for using the
internals.NotErrCodefunction.
|
I'm trying coderabbit but you can mostly ignore the comments unless you think they are important I'll go through the PR and review it tonight. I guess don't feel pressured to obey our AI overlords |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internals/utils.go(2 hunks)string.go(17 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internals/utils.go
🔇 Additional comments (3)
string.go (3)
162-162: LGTM! Well-structured refactoring.The consistent use of
addTestacross all test methods improves maintainability by centralizing the test addition logic.Also applies to: 171-171, 180-180, 189-189, 198-198, 216-216, 235-235, 255-255, 275-275, 295-295, 318-318, 343-343, 370-370, 388-388, 408-408
411-415: LGTM! Clean implementation of the Not operator.The implementation is simple, follows the fluent interface pattern, and aligns well with the PR objectives.
417-424: LGTM! Well-implemented test addition with negation.The method correctly handles test addition with negation and ensures that negation only applies to the next test by resetting the flag.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
string.go (1)
410-414: Add documentation for the Not() method.While the implementation is correct, please add documentation explaining:
- The purpose of the Not() method
- How it affects subsequent test validations
- That it automatically resets after the next test
Add this documentation above the method:
+// Not negates the next test validation. The negation is automatically reset after +// the next test is added. For example: +// schema.Not().Email() // validates that the value is not an email
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
string.go(16 hunks)
🔇 Additional comments (3)
string.go (3)
28-28: LGTM! Well-structured field addition.The
isNotfield is appropriately placed and correctly scoped as a private field.
161-161: LGTM! Consistent and clean refactoring.The test methods have been consistently refactored to use the centralized
addTestmethod, improving maintainability and reducing the risk of inconsistencies.Also applies to: 170-170, 179-179, 188-188, 197-197, 215-215, 234-234, 254-254, 274-274, 294-294, 317-317, 342-342, 369-369, 387-387, 407-407
416-423: Add documentation and verify AddTest implementation.The implementation looks correct, but please add documentation explaining the method's purpose and behavior.
Add this documentation above the method:
+// addTest adds a test to the schema's test list. If isNot is true, the test is +// negated. The isNot flag is automatically reset after adding the test.Let's verify the implementation of
p.AddTest:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the AddTest implementation in internals package # Search for AddTest function definition ast-grep --pattern 'func AddTest($_, $_, $_) $_ { $$$ }'Length of output: 727
Documentation Update and Verification of p.AddTest Implementation
The p.AddTest implementation (found in internals/utils.go) correctly handles both scenarios:
- Appends the test as-is when isNot is false.
- Negates the test's validation function and error code when isNot is true.
Please add the following documentation comment above the addTest method in string.go to clarify its behavior:
+// addTest adds a test to the schema's test list. If isNot is true, the test is +// negated. The isNot flag is automatically reset after adding the test.No issues were found in the p.AddTest implementation.
|
Hey, sorry to have taken so long to reply. I have been thinking about this quite a lot and also been quite busy. These are my thoughts: While this approach works, it has a few gotchas that are not great. Like you can place something like a pre or postTransform after the not and then it will affect the next test. So its not really typesafe. Additionally, this relies on a testFunc API that I think will change slightly as we implement a superRefine like method. I'm also not a fan of having to create 2 error messages for each error due to having the not operator, but I think its something we will have to accept if we decide to do this. So I think we will hold off for now on implementing this until I have implemented the super refine and a few other things. After that we can revisit it. I have also come up with an idea to make the API properly typesafe which is to add a type StringSchema struct {
not *NotStringSchema
}
func (s *StringSchema) Not() {
return s.not
}
type NotStringSchema struct {
parent *StringSchema
}
func (n *NotStringSchema) Contains(x string) {
// add the test to the parent
return n.parent
}This is a little more work for us but will allow us to ensure typesafety |
Actually in that case, can we just defined an interface that only has the methods that has the |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
string_test.go (3)
389-391: Add a test case for combining Not with multiple validations.Consider adding a test case that verifies the behavior when multiple validations are chained after Not(). This would ensure that the isNot flag is correctly reset after applying to a single test.
"not len success": { schema: String().Not().Len(10).Contains("test"), strVal: "test", expectedErrMap: nil, }, +"not with multiple validations": { + schema: String().Not().Len(10).Not().Email().Contains("test"), + strVal: "test", + expectedErrMap: nil, +},
397-405: Update error message for clarity.The current error message "string must not be exactly 4 character(s)" is functional but could be more natural. Consider rewording it to improve readability.
&internals.ZogErr{ C: "not_len", ParamsM: map[string]any{"len": 4}, Typ: "string", Val: "test", - Msg: "string must not be exactly 4 character(s)", + Msg: "string must not be exactly 4 characters long", Err: nil, },
417-424: Add param details to the Not Email error.For consistency with other error messages, consider adding relevant parameters to the not_email error. This makes the error more informative and maintains a consistent pattern across all error types.
&internals.ZogErr{ C: "not_email", - ParamsM: nil, + ParamsM: map[string]any{}, Typ: "string", Val: "test@test.com", - Msg: "must not be a valid email", + Msg: "string must not be a valid email address", Err: nil, },string.go (1)
431-435: Improve documentation for the Not method.The current comment lacks details about how the
Not()method works and which methods can be called after it.-// Test: nots the next test fn +// Not inverts the next validation test, causing it to pass when the condition fails +// and fail when the condition passes. Returns a NotStringSchema that only allows +// validation methods to be called (not modifiers like Required or transformers). func (v *StringSchema) Not() NotStringSchema { v.isNot = !v.isNot return v }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
string.go(17 hunks)string_test.go(2 hunks)
🔇 Additional comments (7)
string_test.go (2)
7-7: Remove redundant import.The
internalspackage is already imported with aliasp. Remove the duplicate import.Apply this diff to fix the redundant import:
- "github.com/Oudwins/zog/internals"
382-450: Good test coverage for the Not operator functionality.The test cases effectively verify both positive and negative scenarios for the Not operator, covering important validations like length checks and email format. The structure with a test table makes it easy to add more test cases in the future.
string.go (5)
13-16: Good use of type assertions to enforce interface compliance.Using interface type assertions at compile time ensures that
StringSchemacorrectly implements both thePrimitiveZogSchemaandNotStringSchemainterfaces.
23-40: Well-designed interface for Not operations.The
NotStringSchemainterface is a good design choice that provides type safety after callingNot(). It properly restricts which methods can be called after negation, preventing misuse of the API.
49-49: Good implementation of the negation state.Adding the
isNotboolean field to track negation state is a clean and effective approach. This allows test methods to toggle their behavior without requiring duplicated logic.
437-443: Ensure isNot is reset after use.The implementation correctly resets the
isNotflag after applying it to a test. This ensures that negation only affects the next test method call, which is the expected behavior.
431-443:❓ Verification inconclusive
Consider edge case of multiple Not() calls.
The current implementation toggles the
isNotflag, which means callingNot()twice would cancel the effect. This might be unexpected for users who expect multiple calls to stack.
🏁 Script executed:
#!/bin/bash # Check if there are any places in the codebase where Not() is called multiple times in sequence rg -p 'Not\(\)\.Not\(\)' --glob '*.go'Length of output: 40
Action: Please review the behavior for handling consecutive
Not()calls.I verified that no instances of chained or immediately consecutive
Not()calls (such asNot().Not()) are present in the codebase. However, note that the current implementation toggles theisNotflag, so callingNot()twice on the same instance will indeed cancel out the effect. It may be worthwhile to either:
- Add tests to explicitly cover this edge case.
- Clarify the intended behavior (possibly in the documentation) if this toggle logic is deliberate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
string_test.go (1)
385-453: Expand negative test coverage for completeness.
These tests effectively validate negated length and email checks. Consider adding more cases (e.g.,Not().URL(),Not().HasPrefix()) to ensure comprehensive coverage of negated conditions.string.go (2)
23-39: Large interface might be further streamlined.
Defining a separateNotStringSchemainterface helps clarify your negated chain usage. However, consider whether embedding or inheriting from another interface could reduce repetition.
49-49: Field name is descriptive; ensure consistent usage.
isNotis straightforward, although a name likenegateNextTestmight be more explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
i18n/en/en.go(1 hunks)internals/utils.go(2 hunks)string.go(17 hunks)string_test.go(2 hunks)
🔇 Additional comments (9)
i18n/en/en.go (2)
4-4: Import usage looks appropriate.
The new import frominternalsis used forNotIssueCode. No issues noted.
10-44: Consider verifying the correctness of negated error messages.
Each negated error message (e.g., "must not be a valid email") aligns with your newNotIssueCodeusage. However, double-check nuances like "must be empty" vs. "must not be empty," ensuring they accurately reflect the intended validation logic.internals/utils.go (3)
5-7: Imports look consistent with usage.
Thestringsandzconstimports are properly utilized for prefix checks and referencing constants.
24-37: Logic for inverting validations is clear and effective.
TheAddTestfunction neatly togglesValidateFuncand adjusts the issue code for negated checks. This design is concise and avoids duplicating code.
39-44: Handle repeated or chained negation carefully.
NotIssueCodetoggles the "not_" prefix if present. While functional, repeated calls can flip back and forth. Ensure this is the intended behavior.string_test.go (1)
7-7: New import is correctly introduced.
This import references your internal utilities. No concerns.string.go (3)
13-16: Compile-time type assertions are correct.
These checks confirmStringSchemaimplements bothPrimitiveZogSchema[string]andNotStringSchema.
448-451: Confirm toggling behavior when calling.Not()multiple times.
TogglingisNotmay cause unexpected behavior in long-chained calls. Verify this meets your intended design.
454-459: ResettingisNotensures each test only gets negated once.
This design is purposeful and prevents accidentally chaining negation across multiple tests.
|
You are a genius. This is much better than what I suggested. I am going to make a change that will affect this to implement a super refine like method. But after that we can consider again. TBH atm I am leaning towards doing it since the interface solution is very clean. |
Closes #41
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests