-
Notifications
You must be signed in to change notification settings - Fork 100
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
Simplify JavaIntegerConstraint structure to align with decimal constraint improvements #1156
Simplify JavaIntegerConstraint structure to align with decimal constraint improvements #1156
Conversation
max = max.min(BIG_INTEGER_MAX_BYTE); | ||
min = min.max(BIG_INTEGER_MIN_BYTE); |
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.
I think this part may be a bit ambiguous. Should we change the variable name?
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.
Sure
negativeMin = defaultIfNull(negativeMin, () -> BigInteger.valueOf(DEFAULT_MIN_NUMBER_VALUE)); | ||
negativeMax = defaultIfNull(negativeMax, () -> BigInteger.ZERO); | ||
} | ||
BigInteger min = negativeMinNumberValue != null |
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.
I think the fields in SimpleValueJqwikPlugin
should be also simplified.
@Nullable
private Long positiveMinNumberValue = null;
@Nullable
private Long positiveMaxNumberValue = null;
@Nullable
private Long negativeMinNumberValue = null;
@Nullable
private Long negativeMaxNumberValue = null;
} | ||
} | ||
|
||
Optional<Digits> digitsAnn = context.findAnnotation(Digits.class); |
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.
It is better not to use the abbreviation to avoid confusion.
digitsAnn
-> digitsAnnotation
} | ||
} | ||
|
||
Optional<Digits> digitsAnn = context.findAnnotation(Digits.class); |
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.
It is better not to use the abbreviation to avoid confusion as well.
@seongahjo
This appears to be an intermittent issue. To investigate further, I increased the test execution from the original 10 attempts to 100,000 attempts locally by modifying the annotation to |
👍 It's a known issue, don't worry about it. |
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.
👍
Summary
This PR simplifies the
JavaIntegerConstraint
structure by removing the separate positive and negative range handling, aligning with the recent decimal constraint implementation improvements in #1131. Additionally, it improves the handling of@Digits
annotation for integer types.Related issues:
Fixes #1153
Description
This PR implements a simplification of the integer constraint structure and improves digit validation:
1. Simplified
JavaIntegerConstraint
Structurepositive
/negative
range handlingmin
/max
range representation:2. Improved
@Digits
Annotation Processing for Integers@Digits
validation behavior with Jakarta Validation's actual implementation@Digits(integer=3)
, correctly allows any number up to 999 (≤ 3 digits)3. Consolidated Constraint Processing
@Min
,@Max
,@Positive
,@Negative
, etc.)4. Reduced Code Complexity
How Has This Been Tested?
Updated
@Digits
annotation tests for integer types to verify correct validation behavior.Is the Document updated?
No documentation update is required as this is an internal implementation improvement that maintains the same behavior from the user's perspective.