Skip to content
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

Fix serious errors in locale/is #3314

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

maranomynet
Copy link

@maranomynet maranomynet commented Jan 3, 2023

See commit messages for more info.

I added a start of a test.ts file for the icelandic locale, focusing on the parsing patterns, which were badly broken.

In addition to fixing typos in the matching and parsing patterns, I also made the matchers way (!) more forgiving/lenient in terms of the "width" of the mathced pattern, to better adhere to the Robustness Principle.

Also: Added proper pluralizations and declensions in formatDistance strings.

@maranomynet maranomynet changed the title Fix locale/is Fix errors in locale/is Jan 3, 2023
- In icelandic day and month names are not capitalized.
- Remove "." from abbr. names to closer match the "en-US" form.
Also use non-capturing groups for better `.match` performance
...especially on the more complex (more lenient) RegExps
@maranomynet maranomynet force-pushed the fix-locale-is branch 2 times, most recently from bb6470a to e313164 Compare January 6, 2023 14:22
@maranomynet maranomynet changed the title Fix errors in locale/is Fix serious errors in locale/is Jan 9, 2023
Copy link
Member

@derekblank derekblank left a comment

Choose a reason for hiding this comment

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

@maranomynet Just catching up here - as the original author of the v1 Icelandic locale, thank you for making these updates for v2 and beyond. I've just left a note about not passing {{count}} to the formatDistance singular params. IMO this would break the convention with other locales where the grammatical declension is hardcoded into the string. Otherwise, this looks good.

src/locale/is/_lib/formatDistance/index.ts Outdated Show resolved Hide resolved
Comment on lines +24 to +42
any: new RegExp(
'^(?:' +
[
/ja(?:n(?:\.|úar)?)?/.source,
/f(?:eb(?:\.|rúar)?)?/.source,
/m(?:r|ar[.s]?)/.source,
/a(?:pr(?:\.|íl)?)?/.source,
/m(?:í|aí\.?)/.source,
/j(?:n|ún[.í]?)/.source,
/j(?:l|úl[.í]?)/.source,
/á(?:gú(?:\.|st)?)?/.source,
/s(?:ept?(?:\.|ember)?)?/.source,
/o(?:kt(?:\.|óber)?)?/.source,
/n(?:óv(?:\.|ember)?)?/.source,
/d(?:es(?:\.|ember)?)?/.source,
].join('|') +
')',
'i'
),
Copy link
Member

Choose a reason for hiding this comment

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

🙇

Copy link
Author

Choose a reason for hiding this comment

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

Thank you.

@maranomynet
Copy link
Author

@derekblank
I just noticed there's an options?.addSuffix section at the bottom of `formatDistance/index.ts, that I hadn't noticed.
This PR does not make it any worse than before, but it's a little broken.

I wouldn't mind fixing it also, but that will take a bit more work, as it involves adding declensions.

I can create a separate PR for that fix, later, or if you don't mind waiting a bit, I'll add it to this one.

@maranomynet
Copy link
Author

I rabbit-holed into fixing the addSuffix option, and here we are.

As you see the grammar is hairy enough already, so I'd prefer not adding "one"-form exceptions all over the place.

What we have now is a grammatically coherent and correct localization for Icelandic — something I've been dying to see released for 8 months now.

@maranomynet
Copy link
Author

maranomynet commented Sep 13, 2023

I'm nost sure why the CI fails to detect describe and it globals for only my test file. I tried to model it exactly after all the other test files in the project.

@derekblank is there something I need to change in the PR?

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.

None yet

2 participants