-
Notifications
You must be signed in to change notification settings - Fork 965
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support to humanizing a TimeOnly as a readable clock notation #1134
Add support to humanizing a TimeOnly as a readable clock notation #1134
Conversation
Here I would say what make sense for the fr culture:
and for the code it would look like (not tested yet):
If this PR is merged I will create a new one to add the above french translation |
src/Humanizer.Tests/ApiApprover/PublicApiApprovalTest.Approve_Public_Api.approved.txt.orig
Outdated
Show resolved
Hide resolved
src/Humanizer/Localisation/TimeToClockNotation/PtBrTimeOnlyToClockNotationConverter.cs
Outdated
Show resolved
Hide resolved
src/Humanizer.Tests.Shared/Localisation/en/TimeToClockNotationTests.cs
Outdated
Show resolved
Hide resolved
src/Humanizer/Localisation/TimeToClockNotation/DefaultTimeOnlyToClockNotationConverter.cs
Outdated
Show resolved
Hide resolved
src/Humanizer.Tests/ApiApprover/PublicApiApprovalTest.approve_public_api.approved.txt
Outdated
Show resolved
Hide resolved
Thanks for all the @hangy ! I removed the de locale from the registry and made the new converters internal. Let me know if you find any more issues with the PR |
What happens to unregistered locale? |
|
No, I mean what happens if someone calls this method with locale for which formatter is not registered? Exception? |
Unless I'm missing something, it would behave like most Humanizer methods, and fall back to English. |
src/Humanizer/Configuration/TimeOnlyToClockNotationConvertersRegistry.cs
Outdated
Show resolved
Hide resolved
Looking at the way the translations work, either on the 5's or quarters, do we think we should add a rounding option so that a time rounds up to the nearest group? So |
@hazzik I run the tests with different cultures and it uses the default one, as @hangy said. @clairernovotny Nice catch there with the registry! I removed them and the tests worked as expected. Also, I think a rounding option is really interesting. I thought about having some And I thought about adding rounding by the previous two minutes and the next two minutes, so anything between return time switch
{
{ Minute: < 05 } => $"{normalizedHour.ToWords()} o'clock",
{ Minute: <= 07 } => $"five past {normalizedHour.ToWords()}",
{ Minute: <= 12 } => $"ten past {normalizedHour.ToWords()}",
{ Minute: <= 17 } => $"a quarter past {normalizedHour.ToWords()}",
{ Minute: <= 22 } => $"twenty past {normalizedHour.ToWords()}",
{ Minute: <= 27 } => $"twenty-five past {normalizedHour.ToWords()}",
{ Minute: <= 32 } => $"half past {normalizedHour.ToWords()}",
{ Minute: <= 37 } => $"twenty-five to {(normalizedHour + 1).ToWords()}",
{ Minute: <= 42 } => $"twenty to {(normalizedHour + 1).ToWords()}",
{ Minute: <= 47 } => $"a quarter to {(normalizedHour + 1).ToWords()}",
{ Minute: <= 52 } => $"ten to {(normalizedHour + 1).ToWords()}",
{ Minute: <= 57 } => $"five to {(normalizedHour + 1).ToWords()}",
{ Minute: > 57 } => $"{(normalizedHour + 1).ToWords()} o'clock"
}; |
38a828b
to
801975d
Compare
@strobelt sounds good to me! Gotta love switch expressions :) |
Great! I made the change and went with the enum to make it more clear when using the converter. |
src/Humanizer.Tests/ApiApprover/PublicApiApprovalTest.approve_public_api.approved.txt
Outdated
Show resolved
Hide resolved
Ping @strobelt? I'd love to get a new release out that includes these changes! |
Hi @clairernovotny! I just updated the PR. Thanks for the heads-up with this |
Add support to humanizing a TimeOnly as a readable clock notation string as asked on #1081
Uses the Converter architecture based on the DateOnly ToOrdinal converter and extension