numbers in spanish and catalan over 100 + small fixes #31
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds long- and short-scale dictionaries and rewrites Catalan and Spanish pronunciation logic to support both scales, updated pluralization/morphology, decimal and infinity handling, and changes function signatures to accept scale flags (and scientific for Catalan). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Pronounce as pronounce_number_ca/es
participant ScaleBuilder as scale selection
participant SubThousand as _sub_thousand
participant ShortScale as _short_scale
participant LongScale as _long_scale
Caller->>Pronounce: number, places[, short_scale, scientific]
Pronounce->>ScaleBuilder: build number_names using chosen scale
alt exact match found
ScaleBuilder-->>Pronounce: direct word
Pronounce-->>Caller: word
else compose needed
alt short_scale == true
Pronounce->>ShortScale: split by 1000s
ShortScale->>SubThousand: render chunk(s)
ShortScale-->>Pronounce: joined phrase
else
Pronounce->>LongScale: split and recurse by large magnitudes
LongScale->>Pronounce: recursive chunk renders
LongScale-->>Pronounce: joined phrase
end
opt decimal or infinity
Pronounce->>Pronounce: append "coma" digits or "infinit"/"menys infinit"
end
Pronounce-->>Caller: final phrase
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
ovos_number_parser/numbers_ca.py (3)
392-395: Simplify boolean comparison and dictionary key checksThe code can be made more Pythonic and cleaner.
Apply this diff to improve code style:
- if short_scale==True: - hundreds = [_SHORT_SCALE_CA[n] for n in _SHORT_SCALE_CA.keys()] + if short_scale: + hundreds = [_SHORT_SCALE_CA[n] for n in _SHORT_SCALE_CA] else: - hundreds = [_LONG_SCALE_CA[n] for n in _LONG_SCALE_CA.keys()] + hundreds = [_LONG_SCALE_CA[n] for n in _LONG_SCALE_CA]
419-422: Consider using ternary operator for better readabilityThe if-else block for setting
_partialcan be simplified.Apply this diff to use a ternary operator:
- if q == 1: - _partial = "cent" - else: - _partial = digits[q] + "-cents" + _partial = "cent" if q == 1 else digits[q] + "-cents"
478-493: Complex morphological adjustments for large numbersThe code handles singular/plural forms and morphological changes for large Catalan numbers. The logic appears correct but could benefit from documentation explaining these linguistic rules.
Consider adding a comment block explaining the morphological rules being applied here, as they are language-specific and might not be obvious to future maintainers:
+ # Catalan morphological adjustments for large numbers: + # - Numbers ending in "rds" get "un" prefix and drop final "s" for singular + # - Numbers ending in "ons" get "un" prefix and change to "ó" ending for singular big_nums = [_LONG_SCALE_CA[a] for a in _LONG_SCALE_CA]ovos_number_parser/numbers_es.py (3)
11-11: Trailing whitespace after 'uno'Minor formatting issue with trailing space.
Apply this diff to remove the trailing whitespace:
- 1: 'uno', + 1: 'uno',
640-643: Simplify boolean comparison and dictionary iterationSimilar style improvements as suggested for the Catalan file.
Apply this diff to improve code style:
- if short_scale==True: - hundreds = [_SHORT_SCALE_ES[n] for n in _SHORT_SCALE_ES.keys()] + if short_scale: + hundreds = [_SHORT_SCALE_ES[n] for n in _SHORT_SCALE_ES] else: - hundreds = [_LONG_SCALE_ES[n] for n in _LONG_SCALE_ES.keys()] + hundreds = [_LONG_SCALE_ES[n] for n in _LONG_SCALE_ES]
753-756: Consider combining nested if statementsThe nested if statements can be combined for better readability.
Apply this diff to combine the conditions:
big_nums = [_LONG_SCALE_ES[a] for a in _LONG_SCALE_ES] - if result in big_nums: - - if result[-4:] == "rdos" or result[-4:] == "ones": - result = "un " + result[:-1] + if result in big_nums and (result[-4:] == "rdos" or result[-4:] == "ones"): + result = "un " + result[:-1]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
ovos_number_parser/numbers_ca.py(3 hunks)ovos_number_parser/numbers_es.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ovos_number_parser/numbers_es.py (2)
ovos_number_parser/numbers_ca.py (4)
_sub_thousand(402-425)_short_scale(427-444)_split_by(446-452)_long_scale(454-471)ovos_number_parser/numbers_it.py (4)
_sub_thousand(819-841)_short_scale(843-858)_split_by(860-866)_long_scale(868-885)
ovos_number_parser/numbers_ca.py (1)
ovos_number_parser/numbers_es.py (4)
_sub_thousand(650-688)_short_scale(697-715)_split_by(717-723)_long_scale(725-745)
🪛 Ruff (0.12.2)
ovos_number_parser/numbers_es.py
640-640: Avoid equality comparisons to True; use short_scale: for truth checks
Replace with short_scale
(E712)
641-641: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
643-643: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
701-701: Yoda condition detected
Rewrite as n >= 0
(SIM300)
718-718: Yoda condition detected
Rewrite as n >= 0
(SIM300)
729-729: Yoda condition detected
Rewrite as n >= 0
(SIM300)
753-755: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
ovos_number_parser/numbers_ca.py
392-392: Avoid equality comparisons to True; use short_scale: for truth checks
Replace with short_scale
(E712)
393-393: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
395-395: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
419-422: Use ternary operator _partial = "cent" if q == 1 else digits[q] + "-cents" instead of if-else-block
Replace if-else-block with _partial = "cent" if q == 1 else digits[q] + "-cents"
(SIM108)
431-431: Yoda condition detected
Rewrite as n >= 0
(SIM300)
447-447: Yoda condition detected
Rewrite as n >= 0
(SIM300)
458-458: Yoda condition detected
Rewrite as n >= 0
(SIM300)
🔇 Additional comments (9)
ovos_number_parser/numbers_ca.py (5)
345-356: Clean implementation of pluralization logicThe revised pluralization rule correctly handles Catalan morphology for words ending with "è", converting them to "ens" for plural forms, and applying standard "s" suffix otherwise.
360-360: Function signature updated to support short and long scale pronunciationsThe addition of
short_scaleandscientificparameters enables proper handling of different number scaling conventions. Good API design choice.
376-379: Proper handling of infinity valuesGood addition of support for positive and negative infinity values with appropriate Catalan translations.
402-425: Well-structured helper function for numbers 0-999The
_sub_thousandfunction correctly handles Catalan number formation rules, including the special case for "vint-i-" pattern and proper hundred forms.
463-463: No action required:_long_scalerecursive calls intentionally useshort_scale=TrueI’ve verified the pattern across all language modules (
numbers_ca.py,numbers_az.py,numbers_cs.py,numbers_en.py,numbers_it.py,numbers_ru.py, etc.): each_long_scaleimplementation invokes itspronounce_number_*helper with the second boolean argument hard-coded toTrue, ensuring that sub-thousand segments are rendered via the short-scale logic within a long-scale context. This is consistent and appears to be the intended design—you can resolve this comment.ovos_number_parser/numbers_es.py (4)
611-611: Well-designed API change for scale supportThe addition of the
short_scaleparameter enables proper support for both numbering conventions used in Spanish-speaking regions.
650-688: Excellent implementation of Spanish number formation rulesThe
_sub_thousandfunction correctly handles all the special cases in Spanish number pronunciation, including:
- Special forms for 22, 23, 26 in the twenties
- Irregular hundreds (100, 500, 700, 900)
- Proper concatenation with "y" for compound numbers
690-695: Proper handling of "uno/un" agreementThe
_un_unohelper function correctly handles the masculine singular form adjustments required in Spanish when numbers precede scale words.
734-734: Verification complete: recursive calls consistently useshort_scale=Trueacross implementationsI’ve confirmed that in the long-scale routines for Spanish, Catalan, and Italian, the recursive calls deliberately pass
Truefor theshort_scaleflag to format sub-million segments using short-scale naming:• In numbers_es.py (line 734):
number = pronounce_number_es(z, places, True)• In numbers_ca.py (line 463):
number = pronounce_number_ca(z, places, True, scientific)• In numbers_it.py (line 877):
number = pronounce_number_it(z, places, True, scientific)This matches the pattern in the Catalan and Italian implementations, confirming the design choice is intentional and consistent. No changes needed.
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ UTG Post-Process Complete No new issues were detected in the generated code and all check runs have completed. The unit test generation process has completed successfully. |
|
Creating a PR to put the unit tests in... The changes have been created in this pull request: View PR |
|
@c-armentano can you check the 2 comments from the review bot and also add a couple unittests? |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
Summary by CodeRabbit
New Features
Bug Fixes