-
Notifications
You must be signed in to change notification settings - Fork 92
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
Bugfix - spanish money converters #152
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #152 +/- ##
=========================================
Coverage 98.70% 98.70%
- Complexity 535 538 +3
=========================================
Files 81 82 +1
Lines 2157 2167 +10
Branches 83 84 +1
=========================================
+ Hits 2129 2139 +10
Misses 14 14
Partials 14 14 ☔ View full report in Codecov by Sentry. |
...est/groovy/pl/allegro/finance/tradukisto/internal/languages/spanish/SpanishValuesTest.groovy
Show resolved
Hide resolved
...ava/pl/allegro/finance/tradukisto/internal/converters/BigDecimalToBankingMoneyConverter.java
Show resolved
Hide resolved
// bugfix - ugly hack for ending with "1" :( numerals + nouns needs to have specific gender form | ||
// for example "51 euros" - it is needed to use "cincuenta y un" instead of "cincuenta y uno" | ||
// converter itself does not have context if it is formatting for "standalone" form or with noun | ||
if (words.endsWith("uno")) { | ||
words = words.replaceAll("uno$","un"); | ||
} |
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.
another option is to create a separate IntegerToStringConverter just for spanish bankingMoney conversion
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.
on the other hand, there's plenty of replaceAll
usages in this project so that wouldn't be an excpetion
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'd go with first proposition but it's up to you - second one is not that ugly in the end.
Hello, when is this correction coming out? Thank you for your response. |
I will take a look today. |
Related to #149