Negative price amount #33
Conversation
There was a problem hiding this comment.
I like a lot of things about the change, but I don’t think it is a good idea to modify the signature of a public function like extract_price_text like this. It may be better to create a new function based on extract_price_text instead. And maybe make it private to avoid an issue like this in the future.
… strings avoiding changes in existing public function return values
|
Besides the fact that However I understand your concern about changing something that may be backward incompatible (specially because I changed the return value of the function), so I renamed it to Please let me know if it is better now! Thanks! |
| price, | ||
| re.VERBOSE | ||
| ) | ||
| negative_amount = bool(negative_amount_search) |
There was a problem hiding this comment.
So we are determining if the amount is negative or positive before we have extracted the amount. I think this may be a problem, for example once #5 is merged, where - 2 items at 24,00 € could parse the price as -24€ (I did not check, though).
There was a problem hiding this comment.
You broke my tests! :-(
You are right. This scenario will fail doing that way. However I don't see how to identify it after extracting the amount. I will include it to the tests and try to solve it.
This PR fixes #29 . It allows the return of negative amount values when needed.