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

"case-syntax" parsing isn't robust. #7

Closed
askvortsov1 opened this issue Apr 5, 2021 · 3 comments · Fixed by #8
Closed

"case-syntax" parsing isn't robust. #7

askvortsov1 opened this issue Apr 5, 2021 · 3 comments · Fixed by #8
Assignees
Labels
enhancement New feature or request question Further information is requested

Comments

@askvortsov1
Copy link
Contributor

Hi! I'm a core developer on the Flarum FOSS Project. We're currently considering options to support ICU MessageFormat syntax in our frontend translator, since we need a consistent API with our backend Symfony translator, which recently dropped a proprietary pluralization format in favor of ICU MessageFormat. I was excited to come across your project after finding alternatives to be absolutely massive in size. I've found a few bugs, and would be glad to contribute fixes.

Consider the following message template:

{gender_of_host, select,
	female {{host} invites you to her party}
	male {{host} invites you to his party}
	other {{host} invites you to their party}
}

If the value given is male, the current algorithm will pick up the suffix of female and return the wrong string. This issue is symptomatic of a greater problem with the current parsing strategy used in pluralTypeHandler and selectTypeHandler:

I've solved this by making a custom parser, and using that to implement the type handlers. I'll open a PR.

Beyond this, there's a few further additions I think would be cool, but wanted to run by you first. I might be able to PR these as well if time allows.

@ultraq
Copy link
Owner

ultraq commented Apr 6, 2021

Hey there! 👋 Thanks for checking out my project and finding some bugs in it.

If the value given is male, the current algorithm will pick up the suffix of female and return the wrong string.

Oh dear 🤦 I'll definitely give your PR a look. You mentioned a custom parser, but it doesn't look like the PR is so huge that it adds a tonne of code except for tests (I know intl-messageformat/react-intl uses a parser under the hood and it makes up for quite a good portion of that library), so that makes me happy.

I'll get to your other points after the PR - you've definitely given me a lot to look at! 😅

@ultraq ultraq closed this as completed in #8 Apr 7, 2021
@ultraq
Copy link
Owner

ultraq commented Apr 7, 2021

(Reopening issue since there's still all those other points to discuss)

@ultraq ultraq reopened this Apr 7, 2021
@ultraq ultraq self-assigned this Apr 7, 2021
@ultraq ultraq added enhancement New feature or request question Further information is requested labels Apr 27, 2021
@ultraq
Copy link
Owner

ultraq commented Sep 9, 2023

OK, probably time I closed this! The original issue about the case parsing has been solved by the linked PRs, and being able to use <...> syntax within messages has been implemented with the OP's projects that builds atop this. I'm also working on a release that includes type definitions so it's not quite TypeScript but it should clear those 'cannot find type declarations' errors in TypeScript projects.

@ultraq ultraq closed this as completed Sep 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants