Conversation
kloor
left a comment
There was a problem hiding this comment.
Thank you very much for this contribution. I've reviewed the code and commented on parts where there were mostly just style changes I would like to see, but all-in-all it works well.
I agree that it is probably best to use an associative array for the dates object to match what the API produces.
I did notice that if I set the from date ahead of the end date (which both default to the current date), the API errors out with this plain text error message:
"from" date cannot be later than "to" date
This is different than any of the /space endpoints, that would provide a JSON-formatted message. This results in the error the client produces being not particularly helpful, so the Client class should be updated to interpret non-JSON error responses as plain text. But, that isn't a necessary change for this pull request.
| return new SpaceAction($this); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Organizationally, this should be before the space() method in this class.
| { | ||
| use TraitIdMultiple; | ||
| use TraitCache; | ||
| use TraitCache; |
| use Lits\LibCal\Exception\ActionException; | ||
|
|
||
| /** Action to get opening hours for a location. */ | ||
| class HoursAction extends Action |
There was a problem hiding this comment.
Entire class should be final instead of individual methods.
| return $result; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
I think the buildDateString() and dateStringIsValid() static methods should just be put in the abstract Action class, so then they could also be used by TraitDate instead of duplicating code.
| * @return bool true if the string is in 'yyyy-mm-dd' format, | ||
| * false if not | ||
| */ | ||
| private static function dateStringIsValid(string $date): bool |
There was a problem hiding this comment.
I'd probably name this isValidDateString() instead.
| public string $parent_lid; | ||
|
|
||
| /** @var DateData[] $dates */ | ||
| public array $dates = []; |
There was a problem hiding this comment.
The type of $dates in the comment should be: array<string, DateData>
|
|
||
| class OpeningHoursData extends Data | ||
| { | ||
| public ?string $from = null; |
There was a problem hiding this comment.
I'd probably make both $from and $to non-nullable as well.
|
|
||
| use Lits\LibCal\Data; | ||
|
|
||
| class OpeningHoursData extends Data |
|
|
||
| use Lits\LibCal\Data; | ||
|
|
||
| class HoursData extends Data |
|
|
||
| use Lits\LibCal\Data; | ||
|
|
||
| class DateData extends Data |
We use both the Spaces and Hours endpoints, so I added access to
/hours.Output looks like:
It passes all tests under PHP7.4.
A couple of points to notice:
The Hours API only has a single endpoint, which makes
Client::hours($id)a little different fromClient::space(). I tried to make it fit in spirit.The response format for the
/hoursendpoint is a bit wonky and isn't documented correctly in LibCal. Specifically, thedatesvalue is not an array, it's an object with date strings as names and date objects as values. This makes it kind of hard to extract DateTime objects for hours without resorting to aHoursData::setDate()function.I decided to stick with the out-of-the-box functionality JsonMapper provides and just represent
datesas an associative array keyed by the string value of the date ('2024-06-02') pointing to an object that contains hours as strings ('11:00am'). Our use case is display and not manipulation, so this was sufficient.And thanks for the library, btw!
Cheers,
Ben