feat: Model added with updated return type and tests - #3
Conversation
…load models Enhances type safety and data handling in the models.
Enhances type safety by returning Upload objects instead of arrays.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the HamroCDN library to use strongly-typed model objects instead of raw arrays, improving type safety and API ergonomics.
Key Changes:
- Introduced three new model classes (Upload, User, File) to represent API responses
- Updated all HamroCDN client methods to return Upload objects instead of arrays
- Refactored tests to use a custom
toBeUploadObjectexpectation and validate model objects
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Models/User.php | New model class representing user data with name and email properties |
| src/Models/File.php | New model class representing file data with url and size properties |
| src/Models/Upload.php | New model class representing upload data with nanoId, user, delete_at, and original file |
| src/HamroCDN.php | Updated methods to return Upload objects; added transformation logic in index() to map arrays to Upload instances |
| src/Contracts/HamroCDNContract.php | Updated interface signatures to reflect Upload return types instead of arrays |
| tests/UploadTest.php | Added custom expectation and model validation test; refactored existing tests to work with Upload objects |
| composer.json | Added nesbot/carbon dependency (unused in current implementation) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * @return HamroCDNObjectWithPagination | ||
| * | ||
| * @throws HamroCDNException |
There was a problem hiding this comment.
The return type documentation is missing for the index() method. While the interface declares @return UploadWithPagination, the actual implementation returns a transformed array structure with Upload objects. Consider adding a return type annotation like @return array{data: array<Upload>, meta: array{total: int, per_page: int, page: int}} to clarify the return structure.
| * @throws HamroCDNException | |
| * @throws HamroCDNException | |
| * @return array{data: array<Upload>, meta: array{total: int, per_page: int, page: int}} |
| } | ||
|
|
||
| /** | ||
| * @throws HamroCDNException |
There was a problem hiding this comment.
Missing return type documentation. Consider adding @return Upload to match the interface documentation style and clarify what is returned.
| * @throws HamroCDNException | |
| * @throws HamroCDNException | |
| * @return Upload |
| expect($upload) | ||
| ->toHaveKey('nanoId') | ||
| ->toHaveKey('user') | ||
| ->toHaveKey('delete_at') | ||
| ->toHaveKey('original'); |
There was a problem hiding this comment.
The custom expectation toBeUploadObject uses toHaveKey() on an Upload object (lines 18-21), but Upload is a class with private properties, not an array. The toHaveKey() expectation will fail when checking for keys on an object. Consider using toHaveProperty() instead, or adjust the expectation to check the getter methods directly.
| expect($upload) | |
| ->toHaveKey('nanoId') | |
| ->toHaveKey('user') | |
| ->toHaveKey('delete_at') | |
| ->toHaveKey('original'); | |
| expect($upload->getNanoId())->not->toBeNull(); | |
| expect($upload->getUser())->not->toBeNull(); | |
| // delete_at can be null, so just check method exists | |
| expect(method_exists($upload, 'getDeleteAt'))->toBeTrue(); | |
| expect($upload->getOriginal())->not->toBeNull(); |
| expect($upload->getOriginal()) | ||
| ->toHaveKey('url') | ||
| ->toHaveKey('size'); |
There was a problem hiding this comment.
The custom expectation uses toHaveKey() on the result of getOriginal() which returns a File object, not an array. This will fail. Consider using toHaveProperty() or checking the getter methods directly (e.g., expect($upload->getOriginal()->getUrl())->toBeString()).
| expect($upload->getOriginal()) | |
| ->toHaveKey('url') | |
| ->toHaveKey('size'); | |
| expect($upload->getOriginal()->getUrl()) | |
| ->toBeString(); | |
| expect($upload->getOriginal()->getSize()) | |
| ->toBeInt(); |
| ->toHaveKey('size'); | ||
|
|
||
| expect($upload->getUser()) | ||
| ->toBeInstanceOf(User::class); |
There was a problem hiding this comment.
The expectation assumes getUser() always returns a User instance, but it can return null according to the Upload model. This will cause a test failure when user is null. Consider using a conditional check: expect($upload->getUser())->toBeInstanceOf(User::class)->or->toBeNull().
| ->toBeInstanceOf(User::class); | |
| ->toBeInstanceOf(User::class)->or->toBeNull(); |
| } | ||
|
|
||
| /** | ||
| * @throws HamroCDNException |
There was a problem hiding this comment.
Missing return type documentation. Consider adding @return Upload to match the interface documentation style and clarify what is returned.
| * @throws HamroCDNException | |
| * @throws HamroCDNException | |
| * @return Upload |
| } | ||
|
|
||
| /** | ||
| * @throws HamroCDNException |
There was a problem hiding this comment.
Missing return type documentation. Consider adding @return Upload to match the interface documentation style and clarify what is returned.
| * @throws HamroCDNException | |
| * @throws HamroCDNException | |
| * @return Upload |
No description provided.