Prioritize numbers next to currencies

[Gallaecio]

Fixes #2, fixes #47

Notes:

• I’m not proud of the readability of the changes, so I would really welcome any architecture/aesthetic feedback.
• What should be done with extract_currency_symbol? Remove, deprecate or keep? It is the current responsible for the coverage drop; if it’s going to stay I should provide tests for it.
• As a side effect of these changes, "From 26 to 50 €" gives 50 instead of 26. Prioritizing the lowest side of a human-readable number range seems worth its own patch, so I changed the test expectations.

[codecov]

Codecov Report

Merging #5 (3fe0965) into master (82c27a8) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master        #5   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines           98       113   +15     
  Branches        21        26    +5     
=========================================
+ Hits            98       113   +15     

Impacted Files	Coverage Δ
price_parser/parser.py	100.00% <100.00%> (ø)
price_parser/_currencies.py	100.00% <0.00%> (ø)

[kmike]

Thanks @Gallaecio! I think the approach you've chosen to fix it makes sense.

What should be done with extract_currency_symbol?

I think it is better to keep it. It can be rather easy to test, as existing pytest.mark.parametrize test cases contain currency.

As a side effect of these changes, "From 26 to 50 €" gives 50 instead of 26. Prioritizing the lowest side of a human-readable number range seems worth its own patch, so I changed the test expectations.

This sounds fine; such strings is what we see in practice, but not what should necessarily be passed to price-parser.

[Gallaecio]

Suggestions applied, along with:

• Tests for extract_currency_symbol keeping current test coverage.
• Tests for the new feature which I had written locally but forgot to commit.
• Comments describing the missing features causing two of the expected failures.

[676339784]

Is this branch still up to date? I'm having the same issue as #47, but it seems that this PR could fix it?

[Gallaecio]

It is not up to date, I need to update it and solve the merge conflicts.

[676339784]

Sounds good! Would it be ready for merging afterwards? Not too familiar with how the whole review process works. Thank you

[Gallaecio]

A maintainer, maybe 2, need to approve the changes.

[676339784]

Awesome.

@kmike Seems like this PR has been in limbo for a while now. Assuming you are a maintainer, does this PR look good to you if it's updated and solved merge conflicts?

[Gallaecio]

@kmike is a maintainer in a lot of projects. Waiting for me to update the pull request and then reviewing it will probably be a better use of his time 🙂

I have a few things I need to prioritize myself before I have time to update this. I hope to handle it sometime next week, but I cannot make any promise.

[676339784]

@kmike is a maintainer in a lot of projects. Waiting for me to update the pull request and then reviewing it will probably be a better use of his time 🙂

Great point.

I hope to handle it sometime next week, but I cannot make any promise.

Thank you! I can also separately attempt bringing this branch up to date and whatnot, but that might not be very fruitful

[676339784]

I saw that you've rebased the branch, are there any procedures I can help with before it gets reviewed? Or if I should ping any maintainers?

[Gallaecio]

There’s nothing else you can do on your side, just patience. When a maintainer has time, it will be reviewed.

[676339784]

Sounds good. Was just wondering since this has been open for almost two years now. Thank you for fixing!

[lopuhin]

Thanks for the PR @Gallaecio , I had a quick look before, and should be able to do the full review early next week.

[Hattorius]

yes

[argaurav]

This PR would help us a lot, Can we do anything to get this through?

[lopuhin]

Hey, thanks for feedback, we'll prioritize this, now we have a much better test suite in master.

[evrial]

In [5]: parse_price("30 options from $299.00")
Out[5]: Price(amount=Decimal('30'), currency='$')

I wish this merged sooner