De ordinals

[Bantolomeus]

I appreciate a review of the code. It feels not right to write down all 32 cases as enum, but german ordinals are partly irregular up to 19. I could not come up with a better solution.

[Bantolomeus]

This PR should solve the german part of #669.

[soywiz]

@Bantolomeus thanks for the PR!

Looking at the code I would suggest a change in how it is done. Let me elaborate on that:

• By using the enum, it is needed to replace some characters to support in getOrdinalInGerman the ä, ö, ü. The enum adds an overhead
• The code ends using GermanOrdinal.values() which generates a new array and then iterate over all the elements.
• The enum elements are not used as literals anywhere, so I would suggest a Map here. To avoid creating a new class, with all the elements, that is normally more heavyweight in some cases.

So my proposal is to do something like:

companion object {
  private val NUM_TO_ORD: Array<String> = arrayOf(
      "", // index 0 (invalid)
      // ...
      "zwölfte", // index 12 (no need to do the replace `oe` to `ö`)
      // ...
  )
  private val ORD_TO_NUM: Map<String, Int> = ORD_TO_NUM.mapIndexed { index, s -> s to index }.toMap()
}

Then accessing the array can be done with ORD_TO_NUM.getOrNull(index) ?: error("Invalid numeral") and the opposite with ORD_TO_NUM[key.toLowerCase()].

[Bantolomeus]

@soywiz I hope deleting companion object : GermanKlockLocale() from the class does not break anything elsewhere, but I was not allowed to define two companion objects in the class.
Thanks for the earlier elaboration.

[soywiz]

Can add some tests covering these German locale ordinals?

[Bantolomeus]

sure

[Bantolomeus]

I will add the tests either later today or tomorrow.

[codecov-commenter]

Codecov Report

Patch coverage: 59.23% and project coverage change: +0.03 🎉

Comparison is base (01def82) 50.95% compared to head (16a8c21) 50.98%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1724      +/-   ##
==========================================
+ Coverage   50.95%   50.98%   +0.03%     
==========================================
  Files        1728     1730       +2     
  Lines      100996   101122     +126     
  Branches    14341    14352      +11     
==========================================
+ Hits        51458    51561     +103     
- Misses      45587    45626      +39     
+ Partials     3951     3935      -16     

Flag	Coverage Δ
unittests	50.98% <59.23%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...n/kotlin/korlibs/image/vector/Context2dArrowExt.kt	0.00% <0.00%> (ø)
.../korlibs/math/geom/vector/VectorBuilderArrowExt.kt	0.00% <0.00%> (ø)
...src/commonMain/kotlin/korlibs/math/geom/Vector2.kt	50.73% <41.17%> (-0.31%)	⬇️
...commonMain/kotlin/korlibs/math/geom/Orientation.kt	68.18% <50.00%> (+7.07%)	⬆️
...a/src/commonMain/kotlin/korlibs/math/geom/Angle.kt	74.82% <66.66%> (-0.56%)	⬇️
...ck/src/commonMain/kotlin/korlibs/time/locale/de.kt	96.36% <94.59%> (-3.64%)	⬇️
...c/commonTest/kotlin/korlibs/math/geom/AngleTest.kt	95.74% <100.00%> (+1.37%)	⬆️
...onTest/kotlin/korlibs/math/geom/OrientationTest.kt	100.00% <100.00%> (ø)
...c/commonTest/kotlin/korlibs/math/geom/PointTest.kt	90.47% <100.00%> (+1.00%)	⬆️

... and 20 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[Bantolomeus]

I could not come up with a good test for the ordinals so I copied your english one. But that is kinda useless, since it does exactly the same as the german ordinal code since I can not add a suffix to the german ordinals.
Please tell me if the test shall stay or if I should remove it.
And I changed the index 0 to "nullste" since you did it that way in english, to stay consistent.
If you have no further concerns you can merge the PR from my end.

[soywiz]

Looks good to me.
Don't remember the english version, but since it is not going to be accessed the first element could be an empty string or a null. Then it could be checked as if (number !in 1..31) outofboundsexception.
In any case, looks good to me as it is now 👍. Thanks for the PR @Bantolomeus !

[Bantolomeus]

Thanks for Guiding me through it. It was my pleasure and my very first open source contribution =D

[Bantolomeus]

The native windows Test failed, but it do not understand what the error is, something with coroutines, but nothing concrete. Was the Test flaky?

[soywiz]

It is flaky indeed. Nothing to do with this code. I have re-run it, and likely will be merged now.