Skip to content

fix(normalize): validate symbol currencies - #940

Open
Mr-Neutr0n wants to merge 3 commits into
semantica-agi:mainfrom
Mr-Neutr0n:fix/normalize-currency-symbol-codes
Open

fix(normalize): validate symbol currencies#940
Mr-Neutr0n wants to merge 3 commits into
semantica-agi:mainfrom
Mr-Neutr0n:fix/normalize-currency-symbol-codes

Conversation

@Mr-Neutr0n

Copy link
Copy Markdown

Summary

  • include every currency code exposed by the symbol parser in the supported-code list
  • keep symbol parsing and validate_currency_code() consistent
  • add a round-trip regression test for all configured currency symbols

Verification

  • uv run --no-project --with pytest --with chardet --with loguru --with tqdm --with numpy --with pandas pytest tests/normalize/test_number_normalizer.py -q
  • 7 passed

This keeps parsed RUB, KRW, ILS, NGN, and PKR values from being rejected by the same normalizer API that produced them.

Signed-off-by: Mr-Neutr0n <harikp2002@gmail.com>
@qodo-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Fix currency code validation for symbol-parsed currencies

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Add missing currency codes (RUB, KRW, ILS, NGN, PKR) to the supported-code allowlist.
• Keep symbol parsing and validate_currency_code() behavior consistent.
• Add a regression test that round-trips all configured currency symbols through validation.
Diagram

graph TD
  A["CurrencyNormalizer"] --> B["Symbol parser"] --> C["Parsed currency code"] --> D["validate_currency_code()"] --> E[("currency_codes allowlist")]
  F["Regression test"] --> A
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Single source of truth for supported codes
  • ➕ Eliminates drift between currency_symbols and currency_codes
  • ➕ Makes future symbol additions automatically pass validation
  • ➖ Slightly reduces explicitness of the supported-code list if it is intended to be curated separately
2. Adopt an ISO-4217 currency library/dataset
  • ➕ More complete/standardized currency coverage
  • ➕ Avoids hand-maintained lists and edge-case omissions
  • ➖ Adds dependency/maintenance overhead
  • ➖ May expand accepted inputs beyond current product intent

Recommendation: Current fix is appropriate and low-risk given the small scope and added regression test. If the intent is that all symbol-mapped currencies must be valid, consider deriving currency_codes (or the validation set) from currency_symbols to prevent future inconsistencies.

Files changed (2) +11 / -0

Bug fix (1) +5 / -0
number_normalizer.pyAdd symbol-mapped currencies to supported currency code allowlist +5/-0

Add symbol-mapped currencies to supported currency code allowlist

• Extends 'CurrencyNormalizer.currency_codes' to include RUB, KRW, ILS, NGN, and PKR. This prevents 'validate_currency_code()' from rejecting codes produced by the symbol parser.

semantica/normalize/number_normalizer.py

Tests (1) +6 / -0
test_number_normalizer.pyAdd round-trip test for symbol currency parsing and validation +6/-0

Add round-trip test for symbol currency parsing and validation

• Introduces a regression test that iterates over all configured currency symbols, normalizes an amount, and asserts the resulting code both matches the expected mapping and passes 'validate_currency_code()'. This locks in consistency between parsing and validation.

tests/normalize/test_number_normalizer.py

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Aug 12, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. False-positive code detection ✓ Resolved 🐞 Bug ≡ Correctness
Description
CurrencyNormalizer.normalize_currency() detects codes via substring search (`if code in
currency_input.upper()`); adding RUB/KRW/ILS/NGN/PKR increases false positives (e.g., "wilson 100"
contains "ILS", "ruby 100" contains "RUB"). This can return an incorrect currency code even when the
code isn’t a standalone token.
Code

semantica/normalize/number_normalizer.py[R565-568]

+            "RUB",
+            "KRW",
+            "ILS",
+            "NGN",
Evidence
The PR adds new 3-letter codes into currency_codes, and normalize_currency() detects codes using
substring containment over the whole input, so any word containing those sequences can now be
misinterpreted as a currency code.

semantica/normalize/number_normalizer.py[552-570]
semantica/normalize/number_normalizer.py[611-626]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`CurrencyNormalizer.normalize_currency()` matches currency codes with a substring check (`if code in currency_input.upper()`), which can misclassify normal words containing a 3-letter code. Adding `RUB/KRW/ILS/NGN/PKR` expands the set of substrings that can trigger this, causing incorrect `currency_code` detection.
### Issue Context
- The supported code list was expanded to include symbol-exposed currencies.
- The parsing logic for currency codes currently uses substring containment, not token/boundary-aware matching.
### Fix Focus Areas
- semantica/normalize/number_normalizer.py[552-570]
- semantica/normalize/number_normalizer.py[611-626]
### Suggested implementation direction
- Replace the substring containment check with boundary-aware matching. For example:
- Tokenize the input into alphabetic runs and match `code` only when a token equals the code (case-insensitive), OR
- Use a regex that prevents matching within longer alphabetic words (e.g., `(?i)(?<![A-Z])USD(?![A-Z])`), while still allowing digits/punctuation adjacent to the code (e.g., `100EUR`, `EUR100`).
- Keep the existing symbol-first behavior unchanged.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread semantica/normalize/number_normalizer.py
Signed-off-by: Mr-Neutr0n <harikp2002@gmail.com>
@Mr-Neutr0n

Copy link
Copy Markdown
Author

Thanks for flagging this. I confirmed the substring matcher could classify words such as ruby and wilson as currency codes after the supported-code list was expanded.

The latest commit uses alphabetic boundaries, while still accepting digit-adjacent forms such as 100RUB. Regression coverage now includes valid RUB inputs and the false-positive word cases.

Verification: the focused number-normalizer command passes 8 tests, and both touched Python files compile cleanly.

@ZohaibHassan16 ZohaibHassan16 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good. Approved.

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