Skip to content

feat: bring Locale to parity with FormatJS and ECMA-402#11

Merged
ramsey merged 3 commits intomainfrom
feature/formatjs-locale-parity
Nov 8, 2021
Merged

feat: bring Locale to parity with FormatJS and ECMA-402#11
ramsey merged 3 commits intomainfrom
feature/formatjs-locale-parity

Conversation

@ramsey
Copy link
Copy Markdown
Contributor

@ramsey ramsey commented Nov 6, 2021

Description

This PR improves Intl.Locale parity with FormatJS and ECMA-402:

Future improvements to bring this into closer parity might include porting the test262 test suite to PHP in order to ensure full compliance with ECMA-402: https://github.com/tc39/test262/tree/master/test/intl402/Locale

Product requirements and context

We desire to create a library that has feature parity with FormatJS and conforms to the familiar ECMAScript 402 specification.

How has this been tested?

PR Checklist

  • I have added tests to cover my changes.

@ramsey ramsey requested a review from a team November 6, 2021 18:08
* @psalm-param "h11" | "h12" | "h23" | "h24" | null $hourCycle
*/
public function __construct(
?string $calendar = null,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Method __construct has 9 arguments (exceeds 4 allowed). Consider refactoring.

Copy link
Copy Markdown
Contributor Author

@ramsey ramsey Nov 6, 2021

Choose a reason for hiding this comment

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

I'm going to leave this as-is for now. Using this class makes the most sense in a PHP 8 world, where you can use named parameters, so you can do things like:

use FormatPHP\Intl;

$locale = new Intl\Locale('en', new Intl\LocaleOptions(
    calendar: 'gregory',
    region: 'US',
));

Furthermore, this class makes even more sense looking forward to a PHP 8.1 world, where the entire class ends up looking like this:

class LocaleOptions
{
    public function __construct(
        public readonly ?string $calendar = null,
        public readonly ?string $caseFirst = null,
        public readonly ?string $collation = null,
        public readonly ?string $hourCycle = null,
        public readonly ?string $language = null,
        public readonly ?string $numberingSystem = null,
        public readonly ?bool $numeric = null,
        public readonly ?string $region = null,
        public readonly ?string $script = null,
    ) {
    }
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Even with named parameters, I think the "critique" of too many parameters still stands in most cases, because it's a potential smell of doing too much in one method or class - however, in this case this is a "config" or "options" type object, where that sort of critique isn't as relevant. The only thing I would consider changing is checking things like do any of these arguments conflict, or ever have a condition where they can't co-exist, in which case you would separate them out into more classes. I don't think that's the case with this.

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.

I agree. This is really a way to provide type safety for the options, since we can't do it with a standard PHP associative array, while in JavaScript (TypeScript actually), you can pass an object that has the same shape as the type defined, and it all works.

i.e.,

new Locale('en', {
    calendar: 'gregory',
    region: 'US'
});

Comment thread src/Intl/Locale.php Outdated
Comment thread src/Intl/Locale.php Outdated
Comment thread src/Intl/Locale.php Outdated
Comment thread src/Intl/Locale.php Outdated
Comment thread src/Intl/Locale.php
Comment thread src/Intl/Locale.php
Comment thread src/Intl/Locale.php
Comment thread src/Intl/Locale.php
@qlty-cloud-legacy
Copy link
Copy Markdown

Code Climate has analyzed commit 0667da8 and detected 0 issues on this pull request.

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

This pull request will bring the total coverage in the repository to 100.0% (0.0% change).

View more on Code Climate.

Comment thread src/Intl/Locale.php
$this->parsedLocale['language'],
$this->parsedLocale['script'],
$this->parsedLocale['region'],
...array_values($this->parsedLocale['variants']),
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.

I'm using array_values() here, due to some confusion Psalm has with array unpacking that will be cleaned up in PHP 8.1. See https://wiki.php.net/rfc/array_unpacking_string_keys

@ramsey ramsey merged commit ad76f32 into main Nov 8, 2021
@ramsey ramsey deleted the feature/formatjs-locale-parity branch November 8, 2021 15:43
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.

2 participants