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

Updates accents on céntimos in Spanish #240

Merged
merged 13 commits into from
Feb 12, 2019
Merged

Updates accents on céntimos in Spanish #240

merged 13 commits into from
Feb 12, 2019

Conversation

ohduran
Copy link
Contributor

@ohduran ohduran commented Feb 6, 2019

Fixes # by Álvaro Durán

Changes proposed in this pull request:

  • Fixes Spanish language

Status

  • READY
  • HOLD
  • WIP (Work-In-Progress)

How to verify this change

This is a cosmetic change: make sure that the accent in 'céntimos' is there when running num2words with to='currency'.

Additional notes

@erozqba
Copy link
Collaborator

erozqba commented Feb 6, 2019

@ohduran thanks for this contribution! You forgot to update the tests, please update them too.

@erozqba
Copy link
Collaborator

erozqba commented Feb 7, 2019

@ohduran thanks for updating the tests, please take a look into test_cli_with_lang_to because looks like Spanish is also used on this test and need to be updated.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 627

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.001%) to 92.61%
Totals Coverage Status
Change from base Build 621: -0.001%
Covered Lines: 4762
Relevant Lines: 5048
💛 - Coveralls

1 similar comment
@coveralls
Copy link

Pull Request Test Coverage Report for Build 627

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.001%) to 92.61%
Totals Coverage Status
Change from base Build 621: -0.001%
Covered Lines: 4762
Relevant Lines: 5048
💛 - Coveralls

@coveralls
Copy link

coveralls commented Feb 8, 2019 •
edited
Loading

Pull Request Test Coverage Report for Build 637

  • 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.611%
Totals Coverage Status
Change from base Build 621: 0.0%
Covered Lines: 4762
Relevant Lines: 5048
💛 - Coveralls

@ohduran
Copy link
Contributor Author

ohduran commented Feb 8, 2019

@erozqba I'm unable to bring this forward; tests pass when using the command, but tox keeps on failing. Could you please point me in the right direction?

Cheers,
Alvaro.

kingbuzzman
kingbuzzman reviewed Feb 8, 2019
View reviewed changes
tests/test_cli.py Outdated
@@ -106,6 +106,6 @@ def test_cli_with_lang_to(self):
output = self.cli.run_cmd(150.55, '--lang', 'es', '--to', 'currency')
self.assertEqual(output.return_code, 0)
self.assertEqual(
output.out.strip(),
"ciento cincuenta euros con cincuenta y cinco centimos"
output.out.strip().decode('utf-8'),
Copy link

Choose a reason for hiding this comment

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

Would it make sense to add locale.getpreferredencoding() here instead of utf-8?

@kingbuzzman
Copy link

kingbuzzman commented Feb 11, 2019 •
edited
Loading

@ohduran @erozqba

When running it locally i can get it to reproduce by doing:

PYTHONIOENCODING=ascii python /num2words/bin/num2words 150.55 --lang es --to currency

It seems like delegator.Command is unsetting the PYTHONIOENCODING and causing it to crash. The issue with what to do when its ascii remains open.

Follow up, add the following inside tox.ini:

[testenv:py27]
setenv =
    PYTHONIOENCODING = UTF-8

@ohduran
Copy link
Contributor Author

ohduran commented Feb 12, 2019

Hi @erozqba, waiting for your approval to merge this PR and have the grammar corrected. Do let me know if there is anything else you need me to do.

Alvaro.

@erozqba erozqba merged commit eef5b03 into savoirfairelinux:master Feb 12, 2019
@kingbuzzman
Copy link

@erozqba can you make a release for 0.5.10 with this in it? Or are you waiting on some other milestone?

@eortiz-tracktik eortiz-tracktik mentioned this pull request May 12, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erozqba erozqba erozqba approved these changes

@kingbuzzman kingbuzzman kingbuzzman approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@ohduran @erozqba @coveralls @kingbuzzman