-
Notifications
You must be signed in to change notification settings - Fork 964
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
added German TimeOnlyToClockNotation #1249
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion! Good to see you're not using „dreiviertel zwölf“ 😜
src/Humanizer.Tests.Shared/Localisation/de/TimeToClockNotationTests.cs
Outdated
Show resolved
Hide resolved
src/Humanizer/Localisation/TimeToClockNotation/DeTimeOnlyToClockNotationConverter.cs
Outdated
Show resolved
Hide resolved
src/Humanizer/Localisation/TimeToClockNotation/DeTimeOnlyToClockNotationConverter.cs
Outdated
Show resolved
Hide resolved
@hangy you can add a culture like "de-DE-BY" with "dreiviertel zwölf" if you like 😜 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the German changes are fine, but I don't know about changes to the public API.
@@ -1985,6 +1985,15 @@ namespace Humanizer.Localisation.Ordinalizers | |||
} | |||
namespace Humanizer.Localisation.TimeToClockNotation | |||
{ | |||
public class static German |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someone that knows the API policies of the project better than me should decide whether this change is appropriate, considering the existing public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it need to be public? if so can you add some docs that explain the use cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to make it internal for now.
@SpocWeb can you rebase |
@SimonCropp sure, will try and also add some explanation to the contended Method(s) over the weekend, thanks for considering this. |
Hi, I don't know why the CI Builds fail. I am not allowed to look at the errors and locally it builds for me in all Configurations. |
Here is a checklist you should tick through before submitting a pull request:
main
branch (more info below)fixes #<the issue number>
build.cmd
orbuild.ps1
and ensure there are no test failures