feat: add not operator #76
Conversation
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
Assessment against linked issues
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 (
|
![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.
Closes #41
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests