-
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
Language bangla #108 #110
base: master
Are you sure you want to change the base?
Language bangla #108 #110
Conversation
please update readme |
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 reviewed it and left few comments
src/test/groovy/pl/allegro/finance/tradukisto/ValueConvertersTest.groovy
Outdated
Show resolved
Hide resolved
src/test/groovy/pl/allegro/finance/tradukisto/internal/languages/bangle/BangleValuesTest.groovy
Outdated
Show resolved
Hide resolved
src/test/groovy/pl/allegro/finance/tradukisto/internal/languages/bangle/BangleValuesTest.groovy
Outdated
Show resolved
Hide resolved
src/main/java/pl/allegro/finance/tradukisto/internal/languages/bangle/BangleValues.java
Outdated
Show resolved
Hide resolved
src/main/java/pl/allegro/finance/tradukisto/internal/languages/bangle/BangleValues.java
Outdated
Show resolved
Hide resolved
I have changed the code, please check it now. |
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.
LGTM
cc @jglaszka
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 have question about test cases (see my comment in BanglaValuesTest.groovy). Also, in Bangla numbers are also separated by two digits as in Hindi?
@@ -45,7 +46,8 @@ public enum MoneyConverters { | |||
SERBIAN_CYRILLIC_BANKING_MONEY_VALUE(serbianCyrillicContainer().getBankingMoneyConverter()), | |||
FRENCH_BANKING_MONEY_VALUE(frenchContainer().getBankingMoneyConverter()), | |||
BULGARIAN_BANKING_MONEY_VALUE(bulgarianContainer().getBankingMoneyConverter()), | |||
DUTCH_BANKING_MONEY_VALUE(dutchContainer().getBankingMoneyConverter()); | |||
DUTCH_BANKING_MONEY_VALUE(dutchContainer().getBankingMoneyConverter()), | |||
BANGLA_BANKING_MONEY_VALUE(banglaContainer().getBankingMoneyConverter()); | |||
|
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.
Please add your language also to the ValueConverters and LongConvertrs files
src/test/groovy/pl/allegro/finance/tradukisto/internal/languages/bangla/BanglaValuesTest.groovy
Outdated
Show resolved
Hide resolved
src/test/groovy/pl/allegro/finance/tradukisto/internal/languages/bangla/BanglaValuesTest.groovy
Show resolved
Hide resolved
src/test/groovy/pl/allegro/finance/tradukisto/internal/languages/bangla/BanglaValuesTest.groovy
Outdated
Show resolved
Hide resolved
@ronisarkarexe Hi, can you fix git conflicts, please? |
@Override | ||
public Map<Integer, GenderForms> baseNumbers() { | ||
return baseNumbersBuilder() | ||
.put(1, "শূন্য") |
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.
Shouldn't it be 0?
.put(0, "শূন্য")
|
||
@Override | ||
public GenderType genderType() { | ||
return GenderType.NON_APPLICABLE; |
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.
needs to be imported:
import pl.allegro.finance.tradukisto.internal.languages.GenderType;
|
||
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.
This class needs to have twoDigitsNumberSeparator
method implemented
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 ran your tests, 64/100 does not passes :( If counting in hindi is similar to bangla, you can get inspired by PR: #112
I will check, thank for mention |
I resolved conflicts in your PR |
@@ -320,4 +321,9 @@ public LongToStringConverter getLongConverter() { | |||
public BigDecimalToStringConverter getBankingMoneyConverter() { | |||
return bigDecimalConverter; | |||
} | |||
|
|||
public static Container banglaContainer() { | |||
TurkishValues values = new BanglaValues(); |
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.
you need to change TurkishValues
to BanglaValues
, something like this:
BanglaValues values = new BanglaValues();
The addition of the Bangla (Bengali) language has been completed.
New language added. reference