negated restrictions #905
negated restrictions #905
Conversation
| @@ -62,6 +65,23 @@ void shouldCreateBasicRestrictionWithNullValue() { | |||
| }); | |||
| } | |||
|
|
|||
| @Test | |||
| void shouldNegateLTERestriction() { | |||
There was a problem hiding this comment.
I should have a test scenario case when we negate twice, I mean negate a Restriction that is already not
There was a problem hiding this comment.
I mean, not remove this one, but include a second one
There was a problem hiding this comment.
I should have a test scenario case when we negate twice, I mean negate a Restriction that is already
not
Good idea. I added tests for negating a negated restriction here:
f5d3bb9
| @@ -71,6 +73,35 @@ void shouldFailIfEmptyRestrictions() { | |||
| .hasMessage("Cannot create a composite restriction without any restrictions to combine."); | |||
| } | |||
|
|
|||
| @Test | |||
| void shouldNegateCompositeRestriction() { | |||
There was a problem hiding this comment.
The same here, the negate, negate scenario.
| boolean newNegation = isNegated; | ||
| Operator newComparison; |
There was a problem hiding this comment.
This is incredibly anal, I know, but can we avoid the use of non-final instance vars here, please?
Also: remind we what version of Java we're targeting? Can we still not use switch expressions? I thought we could now.
There was a problem hiding this comment.
This is incredibly anal, I know, but can we avoid the use of non-
finalinstance vars here, please?
The non-final variable went away after I moved the negation detail to Operator, so this is covered now.
Also: remind we what version of Java we're targeting? Can we still not use
switchexpressions? I thought we could now.
Jakarta EE 11 targets Java SE 21 and Java SE 17.
Jakarta EE 12 will add Java SE 25 (and I assume drop 17 but not really sure).
It would make sense for Data 1.1 to aim for compatibility with all 3 of these Java levels. I don't know of any concerns with switch statements for these levels.
There was a problem hiding this comment.
OK, so we can use switch expressions then, right?
There was a problem hiding this comment.
Yes, I accepted your suggestion to use the more concise -> instead of : and followed that up with a commit to add ; at the end which was also needed. This is also nice because allowed us to get past the code checking police without needing to add the useless default throw exception. It is actually better without the default because if someone adds a new enumerated value to Operator at some point, this method will not compile until they also add the implementation for negating it.
| default: | ||
| newComparison = comparison; | ||
| newNegation = !isNegated; |
There was a problem hiding this comment.
Actually I think it would make more sense to just add NOT_IN and NOT_LIKE operators (they exist in JDQL), and move all this logic to a negated() method on Operator.
There was a problem hiding this comment.
Good idea. That makes it possible for all operations to be negated and then makes sense for negation to move to Operator as you suggested. I added this in commit c480490
Co-authored-by: Gavin King <[email protected]>
If I remember the discussion correctly, the 3 ideas for how to do negation of restrictions were:
notAll/notAny seemed the most awkward to me, and I realized that the first two are basically the same thing, and it would be trivial to have both ways, where Restrict.not(restriction) is a convenience method that invokes restriction.negate().