Skip to content
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

German translation #120

Merged
merged 1 commit into from
Apr 2, 2014
Merged

German translation #120

merged 1 commit into from
Apr 2, 2014

Conversation

ccellar
Copy link
Contributor

@ccellar ccellar commented Apr 1, 2014

This is work in progress
Ready for review

  • Added translations for dates
  • Added translations for time spans
  • Added German locale file
  • Added Tests for past dates and times
  • Added Tests for future dates and times

[InlineData(-3, "vor 3 Stunden")]
[InlineData(-2, "vor 2 Stunden")]
[InlineData(-1, "vor einer Stunde")]
[InlineData(1, "in einer Stunde")]
Copy link
Member

Choose a reason for hiding this comment

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

Cannot read German but google translate says this means 'in an hour'. If that's correct, which I guess it is considering the positive input for the hour, it shouldn't really be on the HoursAgo test or maybe we should rename the method to be Hours so it can fit both past and future entries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct. It missed this, after I've tested some positive input. I've to find a way to test these cases with respect of time zones. Otherwise they will fail, depending on the local time zone. I will remove if for now.

Copy link
Member

Choose a reason for hiding this comment

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

It shouldn't fail. If you send in UTC date, which is what we're doing here, Humanizer compares against the current UTC date & time. Even if you send in local time you can tell Humanizer it's a local time (using optional utcDate argument) and it will compare it against the local time. So it should be ok.

@MehdiK
Copy link
Member

MehdiK commented Apr 1, 2014

Oh, just saw the WIP comment on the description :) I hope you don't mind my premature review then!

@ccellar
Copy link
Contributor Author

ccellar commented Apr 1, 2014

No problem. That was fast. I will make the changes you've requested :)

@ccellar
Copy link
Contributor Author

ccellar commented Apr 1, 2014

I've refrained from adding tests for future dates for now. I've to think about, how to poke with time zone awareness. I've seen you've implemented something regarding this problem (https://github.com/MehdiK/Humanizer/blob/master/src/Humanizer.Tests/DateHumanizeTests.cs#L28)

Maybe it's worth to refactor this out, so that it is usable by other tests (should go into another PR).

Thoughts?

P.S.: I've rebased my changes, so that you've only one commit to merge

@MehdiK
Copy link
Member

MehdiK commented Apr 1, 2014

I agree that the verify method could live in a base class for all DateTimeHumanizeTests and that's needed for other locales too. Would be a great second PR.

@MehdiK
Copy link
Member

MehdiK commented Apr 1, 2014

You think you could add future date tests as part of this PR? It can later be refactored in the second PR to use the verify method.

@ccellar
Copy link
Contributor Author

ccellar commented Apr 1, 2014

Done

MehdiK added a commit that referenced this pull request Apr 2, 2014
@MehdiK MehdiK merged commit 9257279 into Humanizr:master Apr 2, 2014
@MehdiK
Copy link
Member

MehdiK commented Apr 2, 2014

Awesome. Thanks.

I just had a second look at the Verify method and if we need to apply the same rigor on other locales and I don't think it's needed. The Verify method and the original/English DateTimeHumanizeTests verify that DateTime.Humanize works correctly and so all corner cases have to be verified. When that's done, for other locales we only need to verify that formatting rules and the translation are correct: no need to verify the functionality of DateTime.Humanize again. So I don't think the second PR as we discussed yesterday is needed. Sorry. This was hard to see with sleepy eyes yesterday.

@MehdiK
Copy link
Member

MehdiK commented Apr 3, 2014

And that came back and bit me in #123!! So we do need that PR to extract the Verify method and apply it on all localisations because of the one-off error that's worked around in the Verify method :p

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