Skip to content

GH-18345: adding Locale::isRightToLeft. #18351

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

Closed
wants to merge 2 commits into from
Closed

Conversation

devnexen
Copy link
Member

Checks is the locale is written left to right.
It makes sure all the needed likely subtags are included (maximization) then determines the direction by known matches.

Checks is the locale is written left to right.
It makes sure all the needed likely subtags are included (maximization)
then determines the direction by known matches.
@devnexen devnexen marked this pull request as ready for review April 18, 2025 21:57
@devnexen devnexen requested a review from kocsismate as a code owner April 18, 2025 21:57
ZEND_PARSE_PARAMETERS_END();

if (!locale_len) {
locale = (char *)intl_locale_get_default();
Copy link
Member

Choose a reason for hiding this comment

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

How about make the argument nullable and have this behavior when it's null?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is mostly about consistency, end users expect the same behaviour throughout (e.g. see here or here).

Copy link
Member

Choose a reason for hiding this comment

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

I see, understand.
thanks!

@SakiTakamachi
Copy link
Member

This is a new feature, so I'd like to hear other people's opinions.

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

It looks fine. It's a bit weird to have both a standalone function and a static function, but I suppose this is consistent with the stuff that already exists; and there's no policy on how to deal with that?

locale = (char *)intl_locale_get_default();
}

RETURN_BOOL(uloc_isRightToLeft((const char *)locale));
Copy link
Member

Choose a reason for hiding this comment

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

Nit: you don't need the cast to const char*, this will happen implicitly

size_t locale_len;

ZEND_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_STRING(locale, locale_len)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Seems like this could be PATH_STRING because NULL bytes are treated as the end of the string.
Although it seems this is not done in this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

that s something I planned to revisit separately, the whole bunch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[intl] add support for determining whether a locale uses a RTL script
3 participants