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.

Sign up for GitHub

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

Jump to bottom

[ADD] num2words: add traslation to spanish of several currencies #356

Merged
merged 2 commits into from Jan 26, 2021
Merged

[ADD] num2words: add traslation to spanish of several currencies #356

merged 2 commits into from Jan 26, 2021

Conversation

fernandahf
Copy link
Contributor

Changes proposed in this pull request:

  • In spanish, only translated in few currencies but with this change, we have more currencies to use.

Status

  • [X ] READY

How to verify this change

Run test, add three cases of added currencies.

@fernandahf fernandahf changed the title [ADD] num2words: add traslation to spanish of several currencies Add traslation to spanish of several currencies Dec 23, 2020
@fernandahf fernandahf changed the title Add traslation to spanish of several currencies [ADD] num2words: add traslation to spanish of several currencies Dec 23, 2020
@coveralls
Copy link

coveralls commented Dec 23, 2020 •
edited

Pull Request Test Coverage Report for Build 968

  • 640 of 640 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.5%) to 95.849%
Totals Coverage Status
Change from base Build 950: 0.5%
Covered Lines: 6003
Relevant Lines: 6187
💛 - Coveralls

@fernandahf
Copy link
Contributor Author

@moylop260

Could you review this, please?

Regards.

moylop260
moylop260 reviewed Dec 24, 2020
View reviewed changes
Copy link

@moylop260 moylop260 left a comment

Choose a reason for hiding this comment

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

@fernandahf

Could you test in spanish with all odoo currencies, please?
In order to check if we are not missing something

num2words/lang_ES.py Outdated
'PEN': (('sol', 'soles'), ('céntimo', 'céntimos')),
'CRC': (('colon', 'colones'), GENERIC_CENTS),
Copy link

Choose a reason for hiding this comment

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

colón

Suggested change
'CRC': (('colon', 'colones'), GENERIC_CENTS),
'CRC': (('colón', 'colones'), GENERIC_CENTS),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@fernandahf fernandahf force-pushed the master-add-traslation-spanish-currencies-fer branch from 25e6f46 to 6426543 Compare December 26, 2020 18:42
@moylop260
Copy link

@fernandahf

I just ran the following script, check the output too in the same link:

There are errors using a few currencies.

Could you fix it, please?

moylop260
moylop260 reviewed Dec 27, 2020
View reviewed changes
tests/test_en.py Outdated
@@ -131,6 +131,194 @@ def test_to_currency(self):
"four pesos and one cent"
)

self.assertEqual(
Copy link

Choose a reason for hiding this comment

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

the name of the file is test_en.py it is for English.

What about using a *es.py ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You right, it's fixed.

@fernandahf fernandahf force-pushed the master-add-traslation-spanish-currencies-fer branch 2 times, most recently from ab3480e to ae35a48 Compare December 30, 2020 00:24
@fernandahf
Copy link
Contributor Author

@fernandahf

I just ran the following script, check the output too in the same link:

* https://gist.github.com/moylop260/a821b38e9e98e1e9168e715a248bea76

There are errors using a few currencies.

Could you fix it, please?

I added all currencies except following:

  • BAM
  • QTQ
  • SOD

No information found about those currencies.

Regards.

@moylop260
Copy link

  • QTQ

Check the data file directly from odoo:

@fernandahf fernandahf force-pushed the master-add-traslation-spanish-currencies-fer branch from ae35a48 to 3290be5 Compare December 30, 2020 20:49
@fernandahf
Copy link
Contributor Author

  • QTQ

Check the data file directly from odoo:

* https://github.com/odoo/odoo/blob/b7fa0cfad03a8319f03c1aa89eb67635e1200f67/odoo/addons/base/data/res_currency_data.xml#L765

Done

@moylop260
Copy link

@erozqba @toofun666

Could you check it, please?

cc @eilst

luisg123v
luisg123v suggested changes Jan 7, 2021
View reviewed changes
Copy link

@luisg123v luisg123v left a comment

Choose a reason for hiding this comment

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

  • Traslations -> translations
  • The conventions [ADD], [FIX], etc; belon to Odoo, I'd try to conform guidelines from this project instead.

tests/test_es.py Outdated
)

TEST_CASES_TO_CURRENCY_GBP = (
(1.00, 'un libra con cero pence'),
Copy link

Choose a reason for hiding this comment

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

Is "un" instead of "una" expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's not expected. It's fixed.

num2words/lang_ES.py Outdated
'YER': (('rial', 'riales'), ('fils', 'fils')),
'YUM': (('dinar', 'dinares'), ('para', 'para')),
'ZMW': (('kwacha', 'kwachas'), ('ngwee', 'ngwee')),
'ZRZ': (('zaire', 'zaire'), ('likuta', 'makuta')),
Copy link

Choose a reason for hiding this comment

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

Isn't "zaires" the plural?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You right. It's fixed.

@fernandahf fernandahf force-pushed the master-add-traslation-spanish-currencies-fer branch 4 times, most recently from 6300de1 to 2fb7870 Compare January 7, 2021 21:28
@eilst
Copy link

eilst commented Jan 9, 2021

@lgharib Could you check it, please?

eilst
eilst previously approved these changes Jan 9, 2021
View reviewed changes
@fernandahf
Copy link
Contributor Author

@luisg123v

Could you review again, please?

Regards.

@mromdhane mromdhane self-requested a review January 21, 2021 22:48
erozqba
erozqba reviewed Jan 24, 2021
View reviewed changes
Copy link
Collaborator

@erozqba erozqba left a comment •
edited

Choose a reason for hiding this comment

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

@fernandahf I think we don't need to loop over the CURRENCIES_UNA. Could you also please rebase your branch?

num2words/lang_ES.py Outdated Show resolved Hide resolved
@fernandahf fernandahf force-pushed the master-add-traslation-spanish-currencies-fer branch from 199d27d to 2841f48 Compare January 25, 2021 20:25
@fernandahf fernandahf force-pushed the master-add-traslation-spanish-currencies-fer branch from 49ee41f to b0f2ccd Compare January 25, 2021 20:35
@mrodriguezg1991 mrodriguezg1991 merged commit 400917a into savoirfairelinux:master Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moylop260 moylop260 moylop260 left review comments

@luisg123v luisg123v luisg123v requested changes

@erozqba erozqba erozqba approved these changes

@eilst eilst eilst left review comments

@mromdhane mromdhane Awaiting requested review from mromdhane

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@fernandahf @coveralls @moylop260 @eilst @erozqba @luisg123v @mrodriguezg1991