Skip to content

Conversation

@rennerocha
Copy link

This PR fixes #29 . It allows the return of negative amount values when needed.

@rennerocha rennerocha requested a review from kmike February 13, 2020 14:20
Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

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.

@rennerocha
Copy link
Author

@Gallaecio ,

Besides the fact that extract_price_text seems to be a public function, it is never mentioned in the docs, so only users that read the source code of the library would be using it in their project. In my opinion I may consider it as a private function (just rename it to _extract_price_text would be an acceptable solution).

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 _extract_price_amount and made extract_price_text as before just using this new private function.

Please let me know if it is better now!

Thanks!

price,
re.VERBOSE
)
negative_amount = bool(negative_amount_search)
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Author

Choose a reason for hiding this comment

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

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.

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.

API does not handle negative values

2 participants