-
Notifications
You must be signed in to change notification settings - Fork 4
PHPLIB-1381 PHPLIB-1269 Add enum for Sort and TimeUnit #63
Conversation
src/Builder/Type/Sort.php
Outdated
self::Desc => -1, | ||
self::TextScore => ['$meta' => 'textScore'], | ||
self::SearchScoreAsc => ['$meta' => 'searchScore', 'order' => 1], | ||
self::SearchScoreDesc => ['$meta' => 'searchScore', 'order' => -1], |
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.
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.
Minor feedback regarding the enums, but in general LGTM!
src/Builder/Type/Sort.php
Outdated
self::TextScore => ['$meta' => 'textScore'], | ||
self::SearchScoreAsc => ['$meta' => 'searchScore', 'order' => 1], | ||
self::SearchScoreDesc => ['$meta' => 'searchScore', 'order' => -1], |
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.
Does it make sense to include all of these in a single Sort
enum? IIRC searchScore
is only available in $search
but not $sort
, while textScore
is only available in $sort
but not $search
.
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.
Ok, removed.
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.
Do we want to include searchScore
in a different enum or leave it to the task of adding full operator support to $search
? If so, let's make a note in the respective ticket to remind ourselves.
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.
Let's keep the new enum for later in #58
case Hour = 'hour'; | ||
case Minute = 'minute'; | ||
case Second = 'second'; | ||
case Millisecond = 'millisecond'; |
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.
Nit: I'd argue the correct capitalisation would be MilliSecond
. This has the added advantage of PhpStorm suggesting it when typing TimeUnit::ms
in the autocompletion box.
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.
Done. But I don't get any completion for TimeUnit::ms
in PHPStorm. Whereas I have completion for both TimeUnit::Ms
and TimeUnit::MS
with or without this change. Only the upper M matters.
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.
Thanks for confirming - I had hoped a miniscule "m" would already trigger the correct selection. Thinking about this some more, it should probably be "Millisecond" after all, as "MilliSecond" would imply "milli second" as the correct unit, which is definitely wrong. Sorry for the back and forth here. 😨
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.
Reverted.
object( | ||
age: -1, | ||
posts: 1, | ||
), | ||
age: Sort::Desc, | ||
posts: Sort::Asc, |
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.
😍
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.
LGTM!
Fix PHPLIB-1381 and PHPLIB-1269
I decided to not rely on
BackedEnum
, but use a specificDictionary
interface, with a methodgetValue()
so that the value can be different than onlystring
or onlyint
. This is the case forSort::TextScore
.