Skip to content

feat: port @formatjs/icu-messageformat-parser to PHP#19

Merged
ramsey merged 1 commit intomainfrom
feature/icu-messageformat-parser
Dec 10, 2021
Merged

feat: port @formatjs/icu-messageformat-parser to PHP#19
ramsey merged 1 commit intomainfrom
feature/icu-messageformat-parser

Conversation

@ramsey
Copy link
Copy Markdown
Contributor

@ramsey ramsey commented Dec 10, 2021

Description

This is a straight port of the @formatjs/icu-messageformat-parser package, with some clean-up, reorganizing, and light refactoring.

The answer to most "why did you do X this way?" questions is "because that's how it's done in the FormatJS code." I basically took the TypeScript and rewrote it into PHP.

It might help to look at the code in the branch view on GitHub (instead of the files diff on this PR). Focus on the src/Icu/MessageFormat/ and tests/Icu/MessageFormat/ directories. These are brand new in this PR.

For example, take a look at this test (which is the basic gist of how we’ll use the parser): https://github.com/Skillshare/formatphp/blob/feature/icu-messageformat-parser/tests/Icu/MessageFormat/ParserTest.php#L28-L37

Together with this data (that the test linked above is using): https://github.com/Skillshare/formatphp/blob/feature/icu-messageformat-parser/tests/Icu/MessageFormat/ParserTest.php#L66-L78

All the output the parser produces for the above test and data is in this directory as snapshots: https://github.com/Skillshare/formatphp/tree/feature/icu-messageformat-parser/tests/Icu/MessageFormat/__snapshots__

Product requirements and context

If you think it seems like a really weird thing to copy the MessageFormat parser over from FormatJS, you're not wrong. 😁

We want to add the capability to generate pseudo-locales in FormatPHP. However, there is a problem that we can't solve without a parser.

Given the message:

my name is {name}

We want to generate something like:

ṁẏ ńâṁè íś {name}

Notice how {name} is left untouched. That seems rather simple, though. We just look for { and } and exclude anything inside the braces. Right?

Unfortunately, it's not that simple when we have a message like this:

{gender_of_host, select,
  female {
    {num_guests, plural, offset:1
      =0 {{host} <strong>does not</strong> give a party.}
      =1 {{host} invites {guest} to her party.}
      =2 {{host} invites {guest} and one other person to her party.}
      other {{host} invites {guest} and # other people to her party.}}}
  male {
    {num_guests, plural, offset:1
      =0 {{host} <strong>does not</strong> give a party.}
      =1 {{host} invites {guest} to his party.}
      =2 {{host} invites {guest} and one other person to his party.}
      other {{host} invites {guest} and # other people to his party.}}}
  other {
    {num_guests, plural, offset:1
      =0 {{host} <strong>does not</strong> give a party.}
      =1 {{host} invites {guest} to their party.}
      =2 {{host} invites {guest} and one other person to their party.}
      other {{host} invites {guest} and # other people to their party.}}}}

Having a parser gives us the power to convert the above message into a pseudo locale, without mangling the ICU message syntax or the HTML tags.

How has this been tested?

I've copied over all the tests from @formatjs/icu-messageformat-parser to ensure that this parser produces the exact same output.

PR Checklist

  • I have added tests to cover my changes.

@ramsey ramsey requested a review from a team December 10, 2021 07:07
@ramsey ramsey force-pushed the feature/icu-messageformat-parser branch from c7ed83e to 8eaa804 Compare December 10, 2021 07:08
Copy link
Copy Markdown

@qlty-cloud-legacy qlty-cloud-legacy Bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 14790 lines exceeds the maximum allowed for the inline comments feature.

@qlty-cloud-legacy
Copy link
Copy Markdown

Code Climate has analyzed commit 8eaa804 and detected 61 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 61

Note: there is 1 critical issue.

The test coverage on the diff in this pull request is 92.5% (80% is the threshold).

This pull request will bring the total coverage in the repository to 95.9% (-4.0% change).

View more on Code Climate.

* @var string $property
* @var scalar | mixed[] | null $value
*/
foreach (get_object_vars($this) as $property => $value) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Originally, I had foreach ($this as $property => $value) here, since that works just fine, but Psalm and PHPStan really hate it.

@ramsey
Copy link
Copy Markdown
Contributor Author

ramsey commented Dec 10, 2021

The PR diff size of 14790 lines exceeds the maximum allowed for the inline comments feature.

Sorry, codeclimate!

Copy link
Copy Markdown

@xiian xiian left a comment

Choose a reason for hiding this comment

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

🙏

Copy link
Copy Markdown

@jrode jrode left a comment

Choose a reason for hiding this comment

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

🥇

@ramsey ramsey merged commit 38170ba into main Dec 10, 2021
@ramsey ramsey deleted the feature/icu-messageformat-parser branch December 10, 2021 19:26
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.

3 participants