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

feat: Support for localized strings #120

Merged
merged 12 commits into from
Dec 4, 2024
Merged

Conversation

dimtrovich
Copy link
Contributor

@dimtrovich dimtrovich commented Nov 29, 2024

Introduction of translation support.
Until now, all the texts generated by the CLI have been in English. The problem is that some people run commands in other languages (French in my case) and so you get a mixture of English and French, which isn't very cool to see.

image

This subject has already been discussed here #64

With this PR, the texts are translated and the developer can therefore use the language best suited to his project
image

// example

<?php

use Ahc\Cli\Application;
use Ahc\Cli\Translations\Translator;

include './vendor/autoload.php';

Translator::$locale = 'ru';  // en, fr, es, de, ru 
$app = new Application('My Application');

$app->command('rmdir');

$app->handle($_SERVER['argv']);

en (default)
image

fr - French
image

es - Spanish
image

de - German
image

ru - Russian
image

Copy link
Owner

@adhocore adhocore left a comment

Choose a reason for hiding this comment

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

hello @dimtrovich thank you. it definitely is a big and significant work so kudos to that.

however i feel like we shouldn't have it in the library as it is quite niche and a bit away from focus of cli package.
instead can we make it as a plugin (which you can put under your git repo and we can link it up in readme here with instructions for i18n)

@adhocore
Copy link
Owner

adhocore commented Dec 1, 2024

cc @kodie what's your thoughts on it? for me it is big feature &/or change surface to be in the core

@dimtrovich
Copy link
Contributor Author

Hello @adhocore. Sorry for the delay.
The idea of a plugin is a good one, but at the moment the package doesn't offer any mechanism for creating extensions. This idea could be implemented in v2!!!

The other problem I have with this is that it will be quite difficult to capture your hard-coded text and then translate it... There's a bit of text in all the classes and also between the special formatting tags (for example, in the error stack display method, like <red>Thrown In<end/>:).

With this, I think it will be impossible to externalize the translation system. On the other hand, you could remove the translation files and leave just English, and the developer who wants a different language could extend it as I mentioned in the Readme.

@adhocore
Copy link
Owner

adhocore commented Dec 1, 2024

right now there is no way to externalize translation because thats not the focus/usecase of this pkg.

as for this pr, the surface of code change is too big for not necessarily must have feature.

i would like to have it bare minimum like so:

namespace Ahc\Cli;

class Application
{
  public static $locale = 'en';
  public static $locales = [];

  public static function addLocale(string $loc, array $texts, bool $default)
  {
    if ($default) Application::$locale = $loc;
    Application::$locales[$loc] = $texts;
  }
}

function t(string $text, array $args): string
{
  $loc = Application::$locale;
  // EN is already there as original text (no keys needed)
  $txt = Application::$locales[$loc][$txt] ?? $txt;
 
  return sprintf($txt, ...$args);
}

// internally cli pkg uses t(). 
// eg: in https://github.com/adhocore/php-cli/blob/main/src/Input/Command.php#L199
//  throw new InvalidParameterException('Only last argument can be variadic');
// will be
//  throw new InvalidParameterException(t('Only last argument can be variadic'));

externally end users would use it like so:

Application::addLocale('fr', [
  'Only last argument can be variadic' => 'Seul le dernier argument peut être variadique',
], true);

now since there is no translation key in code, we just list them in the readme from:

https://github.com/adhocore/php-cli/blob/b8e089f464a647b0ec43682a3fd2f2a7dba1a81a/src/Translations/en.php

@adhocore
Copy link
Owner

adhocore commented Dec 1, 2024

also the sprintf positional arguments needs to be handled as explained here: https://www.php.net/manual/en/function.sprintf.php#refsect1-function.sprintf-examples

example text: 'The current (%d) is greater than the total (%d).'

@kodie
Copy link
Contributor

kodie commented Dec 1, 2024

I do like the compromise of allowing string translation, but not having the cli package need to maintain the different translations. What @adhocore suggests makes translations work similar to how the styles system works now which is nice. There are built in defaults, but everything can be customized. My one slight update to what @adhocore suggested would be to make it possible to customize the built-in local as well. For example if I would like the "Show help" description on the --help option to say something like "Shows helpful information about a command". I like this because after all the recent updates, I think this might be the one last thing that currently can't be customized.

@dimtrovich
Copy link
Contributor Author

right now there is no way to externalize translation because thats not the focus/usecase of this pkg.

as for this pr, the surface of code change is too big for not necessarily must have feature.

i would like to have it bare minimum like so:

namespace Ahc\Cli;

class Application
{
  public static $locale = 'en';
  public static $locales = [];

  public static function addLocale(string $loc, array $texts, bool $default)
  {
    if ($default) Application::$locale = $loc;
    Application::$locales[$loc] = $texts;
  }
}

function t(string $text, array $args): string
{
  $loc = Application::$locale;
  // EN is already there as original text (no keys needed)
  $txt = Application::$locales[$loc][$txt] ?? $txt;
 
  return sprintf($txt, ...$args);
}

// internally cli pkg uses t(). 
// eg: in https://github.com/adhocore/php-cli/blob/main/src/Input/Command.php#L199
//  throw new InvalidParameterException('Only last argument can be variadic');
// will be
//  throw new InvalidParameterException(t('Only last argument can be variadic'));

externally end users would use it like so:

Application::addLocale('fr', [
  'Only last argument can be variadic' => 'Seul le dernier argument peut être variadique',
], true);

now since there is no translation key in code, we just list them in the readme from:

https://github.com/adhocore/php-cli/blob/b8e089f464a647b0ec43682a3fd2f2a7dba1a81a/src/Translations/en.php

It's true that I'm not a big fan of having free-to-air channels as table keys. However, I like it because at least it means that all the text is directly readable internally.
I'll do an update soon.

@dimtrovich
Copy link
Contributor Author

@adhocore which means that we'll have to remove the embedded translations from the package and leave it up to the developer.

@adhocore
Copy link
Owner

adhocore commented Dec 2, 2024

yes we have to remove and thats what makes this change less disruptive and all-compatible

and no, it's not the only translation system that uses real en texts as keys (https://www.hiddentechies.com/blog/magento-2/magento2-generate-language-translation-csv/)

@kodie
Copy link
Contributor

kodie commented Dec 2, 2024

WordPress also works that way: https://developer.wordpress.org/reference/functions/__/

@kodie
Copy link
Contributor

kodie commented Dec 2, 2024

Just an idea too, once it's all set up, translations could be stored in gists much like I did with colors: https://gist.github.com/kodie/b63881ccf4368e9e00c0c3277779524a that way the tool can still be translated without having to do it manually every time.

@dimtrovich
Copy link
Contributor Author

yes we have to remove and thats what makes this change less disruptive and all-compatible

done

@dimtrovich
Copy link
Contributor Author

Just an idea too, once it's all set up, translations could be stored in gists much like I did with colors: https://gist.github.com/kodie/b63881ccf4368e9e00c0c3277779524a that way the tool can still be translated without having to do it manually every time.

done. https://gist.github.com/dimtrovich/1597c16d5c74334e68eef15a4e7ba3fd

however, it looks like you didn't put it in the doc. it might as well be in the doc so we don't have to look too hard.
if you hadn't mentioned it here I'd never have known about it, even though it's a real goldmine.

/**
* Translates a message using the translator.
*/
public static function translate(string $text, array $args = []): string
Copy link
Owner

@adhocore adhocore Dec 3, 2024

Choose a reason for hiding this comment

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

it could be a simple function t() or __() under namespace Ahc\cli (not method) :)

then can be imported as use function Ahc\Cli\t

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Owner

Choose a reason for hiding this comment

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

oh ok but i had striked this one and left another note 😀

Copy link
Contributor Author

@dimtrovich dimtrovich Dec 3, 2024

Choose a reason for hiding this comment

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

I don't quite understand. Can you please explain?
the function t() simple or a method in the InflectsString trait. because now I don't understand.

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, now I understand better. I hadn't paid attention to the link you provided. I'm going to manage the argument positioning

So, in the case of the t() function, should we leave it or go back to the previous translate method?

Copy link
Owner

Choose a reason for hiding this comment

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

its ok as it is, we just need to consider the sprintf for wrapping up

Copy link
Contributor Author

@dimtrovich dimtrovich Dec 3, 2024

Choose a reason for hiding this comment

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

In this case, shouldn't positioning only be managed for texts with more than one argument?

For example, this is relevant for The current (%d) is greater than the total (%d). but much less so for Command %s not found.

I would like to leave Command %s not found unchanged but change The current (%d) is greater than the total (%d). to The current (%1$d) is greater than the total (%2$d)..

or we'll make everything the same

Copy link
Owner

Choose a reason for hiding this comment

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

ya thats fine

Copy link
Contributor Author

@dimtrovich dimtrovich Dec 3, 2024

Choose a reason for hiding this comment

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

done

gist sample was also updated

Copy link
Owner

@adhocore adhocore left a comment

Choose a reason for hiding this comment

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

for now no need to have t() but the positional sprintf must be handled :)

@dimtrovich
Copy link
Contributor Author

positional sprintf must be handled :)

how? I think this has already been taken into account. 🤔🤔

@kodie
Copy link
Contributor

kodie commented Dec 3, 2024

Just an idea too, once it's all set up, translations could be stored in gists much like I did with colors: https://gist.github.com/kodie/b63881ccf4368e9e00c0c3277779524a that way the tool can still be translated without having to do it manually every time.

done. https://gist.github.com/dimtrovich/1597c16d5c74334e68eef15a4e7ba3fd

however, it looks like you didn't put it in the doc. it might as well be in the doc so we don't have to look too hard. if you hadn't mentioned it here I'd never have known about it, even though it's a real goldmine.

Putting it in the readme is a good idea! I came up with it after the PR was merged in

@adhocore adhocore merged commit 86be16e into adhocore:main Dec 4, 2024
5 of 6 checks passed
@dimtrovich dimtrovich deleted the translations branch December 4, 2024 08:25
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