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

Fixes issue #1 #36

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fixes issue #1 #36

wants to merge 3 commits into from

Conversation

rpalsaxena
Copy link
Contributor

I have made some edits to the parser.py file to get the issue resolved and let the library detect mill, million, billion etc.

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.

Could you add tests?

@@ -31,7 +31,6 @@ def fromstring(cls, price: Optional[str],
Given price and currency text extracted from HTML elements, return
``Price`` instance, which provides a clean currency symbol and
price amount as a Decimal number.

Copy link
Member

Choose a reason for hiding this comment

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

What do you have against paragraphs? 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That might be due to the auto-indentation of Pycharm. We can definitely undo it. 😄

@@ -201,11 +197,18 @@ def extract_price_text(price: str) -> Optional[str]:
m = re.search(r"""
(\d[\d\s.,]*) # number, probably with thousand separators
\s*? # skip whitespace
([m|M,b|B]il\w*)? # check million* or billion*
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@rpalsaxena rpalsaxena Mar 12, 2020

Choose a reason for hiding this comment

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

Thanks @Gallaecio for flagging this!
I'll update it with: ([mMbB]il\w*)?

Let me know if there's still some improvement that needs to be made.

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.

2 participants