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

Improve Fuzzy parsing with invalid strings #316

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

elfassy
Copy link
Contributor

@elfassy elfassy commented Jun 28, 2024

Why

The fuzzy parsing tries to guess the amount from a string. We can get a little more strict about the guesses we're making

What

  • BREAKING CHANGE: In cases where it's not able to make a guess at all, we should return nil instead of 0
  • When we're not able to guess if the delimiter is for thousands or decimals, defer to the currency decimal delimiter

@elfassy elfassy changed the title Fuzzy parsing invalid nil Improve Fuzzy parsing with invalid strings Jun 28, 2024
@elfassy elfassy changed the base branch from main to v3 June 28, 2024 20:18
@elfassy elfassy added the v3 label Jun 28, 2024
Copy link
Contributor

@TayKangSheng TayKangSheng left a comment

Choose a reason for hiding this comment

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

Okay. With this change, we will need to check all the places that calls Fuzzy.parse to check for nil value and handle with a fallback when necessary.

@elfassy
Copy link
Contributor Author

elfassy commented Aug 15, 2024

I'm going to split the nil behaviour in it's own PR. I'd still like to get the other changes in v3

EDIT: pr -> #327

@elfassy elfassy closed this Aug 15, 2024
@elfassy elfassy reopened this Aug 15, 2024
@elfassy elfassy added v4 and removed v3 labels Aug 15, 2024
Base automatically changed from v3 to main August 30, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants