Skip to content
This repository was archived by the owner on Feb 28, 2025. It is now read-only.

PHPLIB-1381 PHPLIB-1269 Add enum for Sort and TimeUnit #63

Merged
merged 5 commits into from
Feb 8, 2024

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Feb 2, 2024

Fix PHPLIB-1381 and PHPLIB-1269

I decided to not rely on BackedEnum, but use a specific Dictionary interface, with a method getValue() so that the value can be different than only string or only int. This is the case for Sort::TextScore.

self::Desc => -1,
self::TextScore => ['$meta' => 'textScore'],
self::SearchScoreAsc => ['$meta' => 'searchScore', 'order' => 1],
self::SearchScoreDesc => ['$meta' => 'searchScore', 'order' => -1],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@alcaeus alcaeus left a 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!

Comment on lines 25 to 27
self::TextScore => ['$meta' => 'textScore'],
self::SearchScoreAsc => ['$meta' => 'searchScore', 'order' => 1],
self::SearchScoreDesc => ['$meta' => 'searchScore', 'order' => -1],
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, removed.

Copy link
Member

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.

Copy link
Member Author

@GromNaN GromNaN Feb 8, 2024

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';
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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. 😨

Copy link
Member Author

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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍

Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@GromNaN GromNaN merged commit 63dd036 into mongodb:0.1 Feb 8, 2024
@GromNaN GromNaN deleted the PHPLIB-1381 branch February 8, 2024 16:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants