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

Fix error in grammatical case for German translations #193

Closed
wants to merge 2 commits into from

Conversation

hertg
Copy link
Contributor

@hertg hertg commented Oct 5, 2020

This fixes the issues mentioned in the original post at #175.

However, the issue still persists when calling formatDuration(Duration) with a precise Duration that contains multiple units inside the same string.

E.g. The following test would still fail:

assertEquals("in 2 Monaten 4 Stunden 2 Minuten",
    prettyTime.format(prettyTime.calculatePreciseDuration(addTime(61, Calendar.DAY_OF_YEAR))));
Expected :in 2 Monaten 4 Stunden 2 Minuten
Actual   :in 2 Monate 4 Stunden 2 Minuten

I don't really understand why this even results in "in 2 months 4 hours 2 minutes", but I'm probably missing something.

That's because with the current API and without the necessary context information, this can't be solved without doing really dirty workarounds.

All the tests I've added run successfully. Funny enough, some of the others tests fail. But that was already the case before I applied my changes. Might be related to my system (?), would be good if anybody could take a look at that.

{
result = getPluralName(d);
}
return result;
}

protected boolean isPlural(final Duration d, boolean round) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a heads up:
I extracted this to a separate method here because I need to do the exact same check inside my custom DeTimeFormat and like to prevent duplicate code. I think it might generally come in handy to have this method available when implementing a custom TimeFormat.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. This is fine. Consumers of individual resource bundles may need this information. They may also wish to override the functionality, which would require some ... fun... but this would at least make things a bit more discoverable. I don't see any problems with exposing this here.

@hertg
Copy link
Contributor Author

hertg commented Oct 6, 2020

It looks like my IntelliJ IDEA did some whitespace-only changes to existing code when i hit the "reformat" shortcut.

In case you are not aware of that:
You can hide the whitespace-only changes on the Github diff GUI, so only the changes that actually matter are highlighted :)

image

@lincolnthree
Copy link
Member

@hertg Thank you very much for this PR, and my apologies for the delay! We've been busy getting our release of the new www.topdecked.com ready for public use, and a few things have slipped by.

Looking at this now.

@lincolnthree
Copy link
Member

Looks great! All tests passing, and I agree with your changes. Merged!

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.

None yet

2 participants