piecash icon indicating copy to clipboard operation
piecash copied to clipboard

Use locale currency

Open flywire opened this issue 3 years ago • 13 comments

flywire avatar Jun 10 '21 11:06 flywire

Test will need changing - https://github.com/sdementen/piecash/blob/master/tests/test_session.py#L97-L128

flywire avatar Jun 11 '21 12:06 flywire

To finish this PR, the tests/doc/changelog need to be adapted accordingly. Do you need support for these 3 points?

sdementen avatar Jun 11 '21 12:06 sdementen

Yes, I will need some support. Those requirements are reasonable and should be outlined in a contributions guide (which I prefer in a community editable wiki). It doesn't seem worthwhile doing docs and tests without an indication the PR is acceptable. I'd expect:

  1. change log would occur as a comment on a release summarising commits
  2. root_account currency documentation would have been updated with change to parameter description

I have very basic experience with python tests. I don't understand how https://github.com/sdementen/piecash/blob/master/tests/test_session.py#L97-L128 is a reasonable test. How can locale be tested when it varies by the system it is run on?

flywire avatar Jun 11 '21 23:06 flywire

The changelog to be updated is https://github.com/sdementen/piecash/blob/master/CHANGELOG.rst (just add a line in the same format as the previous one)

sdementen avatar Jun 12 '21 07:06 sdementen

For the test, you can use the same logic as:

def test_create_book_locale_currency(locale_set):
    result = locales[locale_set]
    with locale_ctx(locale_set):
        assert create_book(...). default_currency.mnemonic==result

(Pseudo code as I am writing from memory... Not in front of my laptop) Is it clearer?

sdementen avatar Jun 12 '21 07:06 sdementen

How's this? Replace https://github.com/sdementen/piecash/blob/master/tests/test_session.py#L97-L128 with:

def test_create_book_locale_currency(result):
    assert create_book().default_currency.mnemonic==result

locale.setlocale(locale.LC_ALL, '')
test_create_book_locale_currency(locale.localeconv()['int_curr_symbol'])

Check previous edit.

flywire avatar Jun 12 '21 12:06 flywire

I have no idea what any or this code means:

@pytest.yield_fixture(params=locales)
def locale_set(request):
    yield request.param

flywire avatar Jun 12 '21 12:06 flywire

Look at the pytest documentation on fixtures

On Sat, Jun 12, 2021, 14:35 flywire @.***> wrote:

I have no idea what any or this code means:

@pytest.yield_fixture(params=locales)def locale_set(request): yield request.param

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/sdementen/piecash/pull/174#issuecomment-860047640, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJ6S5UMFHSSQUNHMH6R7NTTSNH7RANCNFSM46OKO7RQ .

sdementen avatar Jun 12 '21 12:06 sdementen

Thanks. Now I have:

@fixture
def locale_currency():
    locale.setlocale(locale.LC_ALL, '')
    return test_create_book_locale_currency(locale.localeconv()['int_curr_symbol'])


def test_create_book_locale_currency(locale_currency):
    assert create_book().default_currency.mnemonic==locale_currency

flywire avatar Jun 12 '21 13:06 flywire

Maybe better to reuse the fixture of the other test you showed me before. It was running for two different locales.

On Sat, Jun 12, 2021, 15:50 flywire @.***> wrote:

Thanks. Now I have:

@fixturedef locale_currency(): locale.setlocale(locale.LC_ALL, '') curr = test_create_book_locale_currency(locale.localeconv()['int_curr_symbol']) return curr

def test_create_book_locale_currency(locale_currency): result = locale_currency(curr) assert create_book().default_currency.mnemonic==result

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/sdementen/piecash/pull/174#issuecomment-860056203, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJ6S5QGR6AFFE3VSOQ24Q3TSNQ2VANCNFSM46OKO7RQ .

sdementen avatar Jun 12 '21 14:06 sdementen

This isn't a simple test and I think you had better fix it up. Ideally, it should test setting the two existing currencies and "locale".

btw, locale = 'English_United States.1252' and locales = ..."English_United States.1252": "USD" (ie includes 3-letter International currency symbol)

setlocale() "locale may be a string, or an iterable of two strings (language code and encoding)."

>>> import locale, sys
>>>
>>> if sys.platform == "win32":
...     locales = {
...         "English_United States.1252": "USD",
...         "French_France.1252": "EUR",
...     }
... else:
...     locales = {
...         "en_US.UTF-8": "USD",
...         "fr_FR.UTF-8": "EUR",
...     }
...
>>> locale.setlocale(locale.LC_ALL, '')
'English_Australia.1252'
>>> locales[locale.setlocale(locale.LC_ALL, '')] = locale.localeconv()['int_curr_symbol']
>>> print(locales)
{'English_United States.1252': 'USD', 'French_France.1252': 'EUR', 'English_Australia.1252': 'AUD'}

flywire avatar Jun 13 '21 14:06 flywire

https://travis-ci.org/github/sdementen/piecash/jobs/774475394#L278-L279

I don't think travis has a locale ??

flywire avatar Jun 13 '21 23:06 flywire

The error message below hints to a currency with a trailing blank space... "USD " which cannot be found in the currency table ValueError: Could not find the ISO code 'USD ' in the ISO table

On Mon, Jun 14, 2021, 01:42 flywire @.***> wrote:

https://travis-ci.org/github/sdementen/piecash/jobs/774475394#L278-L279

I don't think travis has a locale ??

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/sdementen/piecash/pull/174#issuecomment-860287265, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJ6S5TTXURKLFJZ2CSHUI3TSU66ZANCNFSM46OKO7RQ .

sdementen avatar Jun 14 '21 03:06 sdementen