Change mobile number formatting, update dependencies, add CI#18
Change mobile number formatting, update dependencies, add CI#18geirarne wants to merge 18 commits into
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates the number formatting logic for mobile and organization numbers, upgrades dependencies (including switching to pnpm and biome), and adds a CI workflow for continuous integration.
- Updated formatting functions for phone, account, and organization numbers with a revised grouping strategy
- Upgraded dependencies and configuration changes in package.json, pnpm-workspace.yaml, and related files
- Added a GitHub Actions workflow for linting, building, and testing the project
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| validateOrganizationNumber/* | Minor formatting changes in tests and implementation |
| validateAccountNumber/* | Similar formatting and semicolon style updates |
| pnpm-workspace.yaml | Added dependency build restrictions |
| package.json | Version bump and dependency upgrades including switching from prettier to biome |
| index.js | Updated formatting for error message consistency |
| formatPhoneNumber/* | Refactored phone number formatting functions with updated return value grouping |
| formatOrganizationNumber/* | Simplified organization number normalization and formatting |
| formatAmount/* | Refactored amount formatting logic to incorporate negative handling and locale selection |
| formatAccountNumber/* | Improved account number formatting using normalized strings and a forEach loop |
| biome.json, .vscode/settings.json, .npmrc, .github/workflows/ci.yml | New or modified configuration files to align with dependency and workflow updates |
| README.md | Updated documentation with additional npm badge and formatting details |
Comments suppressed due to low confidence (2)
formatPhoneNumber/index.ts:80
- [nitpick] Consider adding a comment here to explain the rationale behind the 2-2-2-2 grouping for Norwegian phone numbers. This will help future maintainers understand that the grouping reflects the new format as per updated standards.
return `${localPart.substring(0, 2)} ${localPart.substring(2, 4)} ${localPart.substring(4, 6)} ${localPart.substring(6)}`;
formatAccountNumber/index.ts:12
- [nitpick] The forEach loop mutates the normalizedAccountNumber variable, which might reduce code clarity. Consider refactoring this logic (for example, by using a while loop or by slicing the string in a more functional style) to improve readability.
ACCOUNT_CHUNKS.forEach((length, index) => {
There was a problem hiding this comment.
Seems like a good idea to adhere to the NKOM standard. I'll let Katja and Hampus decide on this part.
Nice that you added Biome for formatting and linting, but does not belong to this pull request. Hard to review and makes changes to all files. Every line changed will be linked to the pull request which is irrelevant.
Therefore I suggest you separate the code relevant to the phone number formatting change to it's own pull request and make one or two more for formatting, linting, CI workflows etc.
Number formatting change
Rationale
Better late than never: Norwegian mobile numbers should no longer be formatted
NNN-NN-NNN, but ratherNN-NN-NN-NN, like all other "normal" numbers: From NKOM in 2019:The guidelines from Språkrådet are updated as well.
We have actually had customers complaints about our "wrong" formatting. 🙃
Changes made
I have updated both the tests and simplified the actual formatting code, by removing the special case for mobile numbers.
Dependency upgrades
Rationale
There was (not surprising, given the number of years since last update) quite a few outdated dependencies, ie, running
npm auditdid not return a nice result, including at least one critical vulnerability.The dev environment/tooling was also a bit out of date, compared to best/normal practices here at OBOS.
Changes made
Add CI
Rationale
Since this package is used quite a bit in Vibbo & Styrerommet, it would be nice to improve QA, especially related to publishing the package to npm.
… this is the least important part of my changes, and will require some setup (at least npm secrets as env variables in GitHub) on your end. So, feel free to remove, if you disagree.
… but if we were to take over this repo, I would absolutely add it. 🤷♂️
Changes made
See
.github/workflows/ci.yml