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

Support mutiple strings in same request #14

Conversation

Korbeil
Copy link
Contributor

@Korbeil Korbeil commented Jul 19, 2018

Issues

Content

  • Updating TranslatorService interface with translateArray(array $strings, string $from, string $to): array method.
  • Creating AbstractTranslator to handle default behavior for translateArray() (which is just calling translate() method for all strings).
  • Updating ChainedTranslator with new translateArray() method & updating translate() method.
  • Moving format() method to AbstractTranslator & making it fixing translated string with first letter uppercase when original isn't.
  • Abstracting translator tests & implementing it for all of them.

@Korbeil Korbeil force-pushed the 13-support-mutiple-strings-in-same-request branch from 488124a to 466e4de Compare July 19, 2018 21:03
@Korbeil
Copy link
Contributor Author

Korbeil commented Aug 23, 2018

Any news ?

@Korbeil
Copy link
Contributor Author

Korbeil commented Dec 20, 2018

@Nyholm should we merge this ?

@Korbeil
Copy link
Contributor Author

Korbeil commented Mar 6, 2019

@Nyholm update :)

@welcoMattic
Copy link
Member

May @bocharsky-bw or @Nyholm could review this 1 year old PR? 😉

@bocharsky-bw
Copy link
Member

bocharsky-bw commented Jul 19, 2019

Only @Nyholm can merge commits with failed TravisCI :/ We need to make tests green here first, then I'd be able to merge

UPD: Oh, wait, I even don't have access to this repo 😇

@Korbeil
Copy link
Contributor Author

Korbeil commented Jul 19, 2019

Only @Nyholm can merge commits with failed TravisCI :/ We need to make tests green here first, then I'd be able to merge

Tests are red cause HHVM does not support PHP anymore :) I already fixed it in #20

@bocharsky-bw
Copy link
Member

I see #20 is merged, so could you rebase this PR with master? :)

@Korbeil Korbeil force-pushed the 13-support-mutiple-strings-in-same-request branch from 67b5404 to 67ae8b1 Compare July 19, 2019 08:55
@Korbeil
Copy link
Contributor Author

Korbeil commented Jul 19, 2019

@bocharsky-bw It's red on PHP 5.5, it's probably time to drop it ? What you think about it ?

See #21

@welcoMattic
Copy link
Member

I think we can drop support of PHP 5.5 as its end of life was in 2016 ...

@Korbeil
Copy link
Contributor Author

Korbeil commented Jul 19, 2019

Just saw but, PHP 5.5 does not work since long time on Travis, see https://travis-ci.org/php-translation/translator/builds/548564414 (which was 1 month ago)

@bocharsky-bw
Copy link
Member

Yes, let's drop 5.5 👍

Thank you for providing the PR. I think @Nyholm will be able to merge it soon

@Simperfit
Copy link

@Nyholm Do you have some time to review this please ? it seems that people will benefit from it ;)

@Nyholm
Copy link
Member

Nyholm commented Aug 8, 2019

I honestly not sure if it is beneficial or not. I've been reviewing this back and forth for a while.

It seams like the only thing we do is some refactoring and:

public function translateArray($strings, $from, $to)
    {
        $array = [];
         foreach ($strings as $string) {
            $array[] = $this->translate($string, $from, $to);
        }
         return $array;
    }

Why do we need that loop inside the implementation themselves?

@Korbeil
Copy link
Contributor Author

Korbeil commented Aug 8, 2019

@Nyholm this is only a bootstrap for #13
The idea is to send request with multiple strings to translate by default instead of translating one string by one. After that PR we have to implement this method for each provider but this translateArray will act as compatibility for all provider that don't support it already :)

@Nyholm
Copy link
Member

Nyholm commented Aug 8, 2019

Lets leave this PR for now. It contains too many refactoring and changes not related to the point you are trying to make.

Please create a new PR with a new interface that has the translateArray function and then pick one provider which implement that new interface.

@Korbeil Korbeil closed this Jan 16, 2020
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.

Support mutiple strings in same request
6 participants