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.

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

Support cents, fixes #6 #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

manycoding
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jul 5, 2019

Codecov Report

Merging #14 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master    #14   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines          93     93           
  Branches       21     21           
=====================================
  Hits           93     93
Impacted Files Coverage Δ
price_parser/parser.py 100% <ø> (ø) ⬆️

@@ -80,7 +80,9 @@ def __eq__(self, other):
Example(None, 'AED 8000 (USD 2179)',
'AED', '8000', 8000),
Example(None, '13800 ₶',
'₶', '13800', 13800)
'₶', '13800', 13800),
Example(None, '99¢',
Copy link
Member

Choose a reason for hiding this comment

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

I think 99¢ should result in Price(value=Decimal('0.99'), currency=None), since the cent symbol does not belong to a specific currency.

Copy link
Member

Choose a reason for hiding this comment

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

This one is super-unclear, what to do exactly in such cases, as it may require changing the format. For example, $ also doesn't belong to a specific currency; we're not normalizing this to ISO codes at the moment.

I'm not sure how cases like 1$ 99¢ are handled here - it seems they are not.

Copy link
Contributor Author

@manycoding manycoding Jul 8, 2019

Choose a reason for hiding this comment

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

I'm not sure how cases like 1$ 99¢ are handled here - it seems they are not.

They are not.

So how we can find what should be what?

Copy link
Member

Choose a reason for hiding this comment

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

Given the current API, what about aiming for:

  • 99¢Price(value=Decimal('0.99'), currency=None)
  • 1$ 99¢Price(value=Decimal('1.99'), currency='$')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1$ 99¢ Is it a real example? Have we seen this before?

Copy link
Member

Choose a reason for hiding this comment

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

That’s a great question. I assumed it was, but I’ve been searching for a few minutes and all I’ve been able to find are suggestions to use ¢ only up to 99, and never in combination with a regular currency symbol.

Copy link
Member

Choose a reason for hiding this comment

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

Should we still aim for 99¢ → Price(value=Decimal('0.99'), currency=None)? Or do you guys have reservations with that approach?

Copy link
Member

Choose a reason for hiding this comment

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

@Gallaecio from what I can see, this would be the most correct interpretation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants