-
Notifications
You must be signed in to change notification settings - Fork 89
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
Add spanish support #111
Add spanish support #111
Conversation
Hi, tomorrow I will look at your PR :) |
Oh, sorry, I was busy and forgot to check your PR. @bartlomiejgraczyk will review your code. |
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 checked test cases (they looks good) and added some comments, more detailed review in core logic will be done by Bartłomiej.
src/main/java/pl/allegro/finance/tradukisto/MoneyConverters.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public String currency() { | ||
return "$"; |
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 it should be €, as it is official currency for Spain
src/main/java/pl/allegro/finance/tradukisto/internal/Container.java
Outdated
Show resolved
Hide resolved
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.
Thank you for your contribution. I'm leaving some remarks. Try to simplify code a little bit, make it more readable. I'm looking forward to your response.
README.md
Outdated
* 🇸🇰 Slovak | ||
* 🇺🇦 Ukrainian | ||
* 🇷🇸 Serbian (Latin) | ||
* 🇷🇸 Serbian (Cyrillic) | ||
* 🇸🇰 Slovak | ||
* 🇪🇸 Spanish | ||
* 🇹🇷 Turkish | ||
* 🇺🇦 Ukrainian |
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.
👍🏻
src/main/java/pl/allegro/finance/tradukisto/internal/Container.java
Outdated
Show resolved
Hide resolved
src/main/java/pl/allegro/finance/tradukisto/internal/languages/spanish/SpanishValues.java
Outdated
Show resolved
Hide resolved
src/main/java/pl/allegro/finance/tradukisto/internal/languages/spanish/SpanishValues.java
Show resolved
Hide resolved
src/main/java/pl/allegro/finance/tradukisto/internal/Container.java
Outdated
Show resolved
Hide resolved
...pl/allegro/finance/tradukisto/internal/languages/spanish/SpanishIntegerToWordsConverter.java
Outdated
Show resolved
Hide resolved
...l/allegro/finance/tradukisto/internal/languages/spanish/SpanishThousandToWordsConverter.java
Outdated
Show resolved
Hide resolved
...l/allegro/finance/tradukisto/internal/languages/spanish/SpanishThousandToWordsConverter.java
Outdated
Show resolved
Hide resolved
...l/allegro/finance/tradukisto/internal/languages/spanish/SpanishThousandToWordsConverter.java
Outdated
Show resolved
Hide resolved
...l/allegro/finance/tradukisto/internal/languages/spanish/SpanishThousandToWordsConverter.java
Outdated
Show resolved
Hide resolved
Hi @luismateoh, we added some comments for you in review. Can you take a look? |
# Conflicts: # README.md # src/main/java/pl/allegro/finance/tradukisto/MoneyConverters.java # src/main/java/pl/allegro/finance/tradukisto/ValueConverters.java # src/main/java/pl/allegro/finance/tradukisto/internal/Container.java
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## master #111 +/- ##
============================================
- Coverage 99.19% 98.46% -0.73%
- Complexity 425 473 +48
============================================
Files 67 71 +4
Lines 1736 1895 +159
Branches 50 73 +23
============================================
+ Hits 1722 1866 +144
- Misses 7 17 +10
- Partials 7 12 +5
|
I carried out most of the suggestions you made, I appreciate the comments you made and agree with them |
I implemented
Integer
converter for spanish.Could you kindly take a look at this pull request? I'm available for discussions.