Skip to content

Improve currency symbol detection #185

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

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

Conversation

RubenIgnacio
Copy link

The version v1.13.0 of monetize currently detects limited currency symbols. For example:

Expected behavior:

Monetize.parse('S/10') # <Money fractional:1000 currency:PEN>
Monetize.parse('PEN10') # <Money fractional:1000 currency:PEN>

Current behavior:

Monetize.parse('S/10') # <Money fractional:1000 currency:USD>
Monetize.parse('PEN10') # <Money fractional:1000 currency:USD>

Proposed Solution

The Money::Currency.table can be used to retrieve the list of currency symbols. In cases where symbols are repeated, the disambiguate_symbol can be used as the symbol. However, this solution has some limitations:

  • Symbols that contain dots may be confused with decimal marks.
  • Certain symbols can be misinterpreted as multiplier suffixes (e.g., "K" for thousands).

To address these limitations, such symbols will be ignored during parsing. Additionally, any problematic symbols can be added to a list of ignored symbols to prevent issues. This can be a starting point but can be refined further to handle more cases and support additional symbols in the future.

@semmons99
Copy link
Member

we are having an issue with CI being automatically run. if someone figures it out, or we get to it ourselves, we can merge this (and all other PRs) right away.

@RubenIgnacio
Copy link
Author

what's the issue with the CI? maybe I can help

@semmons99
Copy link
Member

the workflows all stopped being detected. no tests are running across any repo

@RubenIgnacio RubenIgnacio force-pushed the improve-currency-symbol-detection branch from 5cddd04 to 014c5c9 Compare April 28, 2025 17:25
@RubenIgnacio
Copy link
Author

i just did a test and the workflow ran okay. i understand that for PRs coming from forks for the first time, it requires reviewer approval before running workflow tests. could that be the issue you're encountering, or do you have another workflow problem?

@semmons99
Copy link
Member

semmons99 commented Apr 28, 2025 via email

@RubenIgnacio
Copy link
Author

sure, I can take a look

@semmons99
Copy link
Member

@RubenIgnacio you are now an owner, though I think I may have fixed them...

@RubenIgnacio
Copy link
Author

thanks for the access. did you find the problem? we could try creating a new branch, setting up the workflow there, and sending a PR with any change to test, so we don't affect main. so far, I think it could be a GH problem, because as you mentioned, the tests are failing in all repos, and I haven't found anything strange in the config.

@semmons99
Copy link
Member

semmons99 commented Apr 30, 2025 via email

@RubenIgnacio
Copy link
Author

@semmons99 Hi, I tested in another branch with a friend and the CI works. I think now we just need to be careful if it happens again.

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