make ComparableExpressions sortable#1455
Conversation
| @Deprecated(since = "1.1", forRemoval = true) | ||
| public Sort(String property, boolean isAscending, boolean ignoreCase) { |
| public record Sort<T>(String property, | ||
| boolean isAscending, | ||
| boolean ignoreCase, | ||
| Nulls nullOrdering) { | ||
| public final class Sort<T> { |
njr-11
left a comment
njr-11
left a comment
There was a problem hiding this comment.
I'd rather not introduce Sortable. It doesn't do anything useful for the end user and I don't think we need it. I wanted to try out adding the asc/desc methods to ComparableExpression and the ascIgnoreCase/descIgnoreCase methods to TextExpression. This seems to work fine in a commit of 1400 where I tried it out. I created a couple of new unit tests to verify it as well as updating the test that were previously under 1400 so they test navigable expressions rather than navigable attributes. That all seemed to work.
| default Sort<T> asc() { | ||
| return Sort.asc(this); | ||
| } |
There was a problem hiding this comment.
So @njr-11 the thing I would still defend here after our last conversation is I think this is a much more elegant way to construct a Sort.
There was a problem hiding this comment.
I agree Sort.asc(expression) is more elegant than using a constructor, but this is internal usage and the end user already has a better option in directly using the metamodel. Nonetheless, I added it to the other PR given that I don't see anything objectionable about it.
2 participants
This had a bigger impact than I expected.
Question: do we need
SortableExpression?