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

Regression from #2131 #2151

Closed
tarkah opened this issue Mar 15, 2021 · 4 comments
Closed

Regression from #2131 #2151

tarkah opened this issue Mar 15, 2021 · 4 comments

Comments

@tarkah
Copy link

tarkah commented Mar 15, 2021

Related to #2131

This change seems to break when you have thousands separator AND decimal separator...

source from amazon id="priceblock_ourprice": $1,799.00

replace = $1,799.00 (doesn't actually do anything?)
match = ["1", "799", 00")
join = 1.799.00
parseFloat = 1.799

Also, the regex shouldn't be "escaped" here. It's actually doing nothing (matching on literal \ and . or literal \ and ,. However, even if it was replacing correctly, the number would parse as 179900 which is also incorrect.

Originally posted by @tarkah in #2131 (comment)

@starcraft66
Copy link

I think I just ran into this issue. The latest nightly of streetmerchant is telling me that this card is selling for $2.498 when it is in fact for sale for $2,498.99 on the website.

@tarkah
Copy link
Author

tarkah commented Mar 16, 2021

#2153 has a fix. However, I feel like the old way of defining the price in the model as "euro" and having much simpler logic that we know will work, was the better solution. There is still a "hack" in #2153 that is sane, but will still potentially miss things.

@jef
Copy link
Owner

jef commented Mar 17, 2021

Agreed @tarkah. I might revert if it continues to cause problems. I'll close this for now and reopen if we see this becoming a problem.

@jef jef closed this as completed Mar 17, 2021
@jef
Copy link
Owner

jef commented Mar 17, 2021

I ended up reverting everything, didn't want to hold this off much longer. We can revisit this when I have more time to test.

That being said, I have mocha and sinon as dev dependencies. Something like this in the future should really have testing around it.

Refs: 0ff8158, 91c4f12

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

No branches or pull requests

3 participants