Adds toString method to Alpha2Code, Alpha3Code, and AlphaCode classes for better string representation.#22
Conversation
…lasses for better string representation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44756e4b9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR adds toString() methods to the AlphaCode interface and its implementations (Alpha2Code, Alpha3Code), allowing for easy string representation of country codes. Additionally, it modifies the Timezones.findByIdentifier() method to return a non-nullable Timezone object (with UTC as fallback) instead of returning null when a timezone is not found.
Changes:
- Added
toString()method to AlphaCode interface and all implementations to return the alpha code value as a string - Changed
Timezones.findByIdentifier()from nullable to non-nullable return type, falling back to UTC when a timezone is not found - Added comprehensive test coverage for the new
toString()functionality - Updated existing tests to align with the new findByIdentifier behavior
- Applied trailing comma formatting to data provider arrays for consistency
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/AlphaCode.php | Added toString() method to the interface definition |
| src/Alpha2Code.php | Implemented toString() method returning the 2-letter code value |
| src/Alpha3Code.php | Implemented toString() method returning the 3-letter code value |
| src/Timezones.php | Changed findByIdentifier() return type from nullable to non-nullable with UTC fallback |
| tests/Models/AlphaCodeXpto.php | Added toString() implementation to test enum for interface compliance |
| tests/CountryTest.php | Added new test for toString(), updated tests for findByIdentifier change, added trailing commas, reordered data providers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public function findByIdentifier(string $iana): Timezone | ||
| { | ||
| return array_find( | ||
| $this->items, | ||
| static fn(Timezone $timezone): bool => $timezone->value === $iana | ||
| ); | ||
| ) ?? Timezone::utc(); | ||
| } |
There was a problem hiding this comment.
The signature of findByIdentifier has been changed from returning Timezone|null to returning Timezone (with UTC fallback). However, the README documentation still shows the old behavior where null is returned when a timezone is not found (line 192: $country->timezones->findByIdentifier(iana: 'Asia/Tokyo'); # null). The documentation should be updated to reflect that UTC is now returned instead of null in this case.
| public function findByIdentifier(string $iana): Timezone | ||
| { | ||
| return array_find( | ||
| $this->items, | ||
| static fn(Timezone $timezone): bool => $timezone->value === $iana | ||
| ); | ||
| ) ?? Timezone::utc(); | ||
| } |
There was a problem hiding this comment.
This is a breaking API change. Code that previously relied on checking for null when a timezone is not found will no longer work as expected. Consider whether this breaking change is intentional and properly communicated. If the behavior is changed, consumers who were checking for null (if ($timezone === null)) will now always receive a Timezone object (UTC), which could lead to silent bugs in existing code that expects to handle the null case differently.
| public function findByIdentifier(string $iana): Timezone | ||
| { | ||
| return array_find( | ||
| $this->items, | ||
| static fn(Timezone $timezone): bool => $timezone->value === $iana | ||
| ); | ||
| ) ?? Timezone::utc(); | ||
| } |
There was a problem hiding this comment.
The PR title only mentions adding toString methods, but this PR also includes a significant breaking change to the Timezones.findByIdentifier method (changing from returning null to returning UTC when a timezone is not found). This breaking change should be prominently mentioned in the PR description to ensure reviewers and users are aware of the behavioral change and its implications.
No description provided.