Skip to content

fix: implement dayPeriod and ensure hour12 functions properly#45

Merged
ramsey merged 2 commits intomainfrom
fix/day-period
Feb 7, 2022
Merged

fix: implement dayPeriod and ensure hour12 functions properly#45
ramsey merged 2 commits intomainfrom
fix/day-period

Conversation

@ramsey
Copy link
Copy Markdown
Contributor

@ramsey ramsey commented Feb 5, 2022

Description

The dayPeriod property was an oversight, so this implements it, while fixing a logic error in the hour12 functionality.

Product requirements and context

How has this been tested?

PR Checklist

  • I have added tests to cover my changes.

@ramsey ramsey requested a review from a team February 5, 2022 02:21
@qlty-cloud-legacy
Copy link
Copy Markdown

Code Climate has analyzed commit 2f46bb5 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 97.4% (0.1% change).

View more on Code Climate.

@ramsey ramsey requested a review from jmauerhan February 7, 2022 14:25
Copy link
Copy Markdown

@jmauerhan jmauerhan left a comment

Choose a reason for hiding this comment

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

I do not envy you working on this, breaks my brain a bit :D


break;
case 'b': // am, pm, noon, midnight
if ($length === 5) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What is length here? what is 5?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good question. 5 is the magic number for "narrow" in this documentation: https://unicode-org.github.io/cldr/ldml/tr35-dates.html

It would be good to try to abstract this into a constant. However, I think this entire class needs to be refactored. In its current form, it's a straight port of JavaScript code to PHP, which is why it looks the way it does. I would never have written it like this to begin with. 😉

@ramsey ramsey merged commit b95a718 into main Feb 7, 2022
@ramsey ramsey deleted the fix/day-period branch February 7, 2022 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants