-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Portuguese support + server tests #7
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
Conversation
- Added Portuguese (pt) language with full number-to-words support - Added 34 server API tests with supertest - Updated README to document 9 languages and REST API - Fixed server to return 400 for invalid inputs (not 500) - Added Russian to server /languages endpoint Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6dcfe3e152
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
index.js
Outdated
| import { english, spanish, french, german, danish, chinese, hindi, russian, portuguese, LANGUAGES } from './languages/index.js'; | ||
|
|
||
| // Re-export language functions | ||
| export { spanish, french, german, danish, chinese, hindi, russian }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Export portuguese as a named language
The new portuguese function is imported but not re-exported alongside the other language helpers, so import { portuguese } from 'numberstring' will fail even though the README and API imply it exists. This is a user-facing API regression specific to the new language: only toWords(..., { lang: 'pt' }) works, but the named export does not.
Useful? React with 👍 / 👎.
languages/pt.js
Outdated
| // Use "e" (and) for connecting in Portuguese | ||
| const prevPart = parts[i - 1]; | ||
| const currPart = parts[i]; | ||
| // Add "e" when the current part is less than 100 or ends with 00 | ||
| if (currPart && !currPart.includes('mil') && !currPart.includes('ilhão') && !currPart.includes('ilhões')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Insert "e" between higher groups and thousands
Portuguese uses “e” between a higher-scale group and the thousands group (e.g., 1,001,000 → “um milhão e mil”). The join logic explicitly skips inserting “e” when the current part contains mil, which yields outputs like “um milhão mil” for any number where the thousands group is non-zero and a higher group exists. This produces incorrect wording for common inputs such as 1,001,000 or 2,005,000.
Useful? React with 👍 / 👎.
CI runs tests from the root, so server test dependencies need to be in the main package.json. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add portuguese to named exports in index.js (Codex review) - Fix Portuguese join logic to correctly use "e" between higher scale groups (milhão/bilhão) and thousands (mil) - Add comprehensive Portuguese test suite with 8 test cases - Add toWords test for Portuguese language Fixes the issue where "um milhão mil" was output instead of the correct "um milhão e mil" for 1,001,000. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add PR review workflow section with guidance on checking comments - Add supported languages list (9 languages now supported) - Add features section documenting full capabilities - Emphasize running lint/tests before commits as first step Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Both Codex suggestions have been addressed in commit
Thanks @codex for the helpful review! |
|
To use Codex here, create an environment for this repo. |
Summary
Test plan
🤖 Generated with Claude Code