Skip to content

Add Meta Informations to the common Message object #98

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

Merged
merged 4 commits into from
Jun 1, 2017

Conversation

damienalexandre
Copy link
Contributor

See php-translation/loco-adapter#7 and FriendsOfApi/localise.biz#16.

This PR adds Meta informations from the Data Collector Message to the Common Message object. This allow the Loco-Adapter to use this Meta information in order to add the parameters as notes on newly created assets.

Screenshot

image

@Nyholm Nyholm self-requested a review May 14, 2017 17:00
$this->domain,
$this->locale,
$this->translation,
$meta
Copy link
Member

Choose a reason for hiding this comment

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

Seems better to me to add parameters, count and transChoiceNumber to the Message value object as fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, I'm not sure: the Message object is a common objet used to represent a translation on the storage, and those parameters, count and transChoiceNumber information are only needed when displaying a translation.

@damienalexandre
Copy link
Contributor Author

Do not merge I still have a cleanup to do:

image

@Nyholm
Copy link
Member

Nyholm commented May 31, 2017

Great. This looks good. I merged the two downstream PRs and tagged new releases.

@damienalexandre
Copy link
Contributor Author

Clean up done, as the profiler was storing all the parameters from all the calls to a translation, the dumped version could become heavy; I reduce the array to only have one occurrence of each parameter key.

@damienalexandre damienalexandre force-pushed the addMetaToMessages branch 2 times, most recently from 556a6a4 to d9b16a1 Compare May 31, 2017 17:50
@Nyholm
Copy link
Member

Nyholm commented May 31, 2017

Feel free to drop support for HHVM. Symfony has officially dropped it.

@damienalexandre
Copy link
Contributor Author

HHVM dropped, but there is a PHP7 syntax error now 😬 https://travis-ci.org/php-translation/symfony-bundle/jobs/238027526
Can't find it!

@Nyholm
Copy link
Member

Nyholm commented May 31, 2017

Two things:

  1. Twig fucked up

  2. We are installing dev and not releases. We should use:

     - travis_retry composer update ${COMPOSER_FLAGS} --prefer-dist --no-interaction		

@damienalexandre
Copy link
Contributor Author

Launched the builds again and it's green 👍

@Nyholm Nyholm merged commit 5b5fb76 into master Jun 1, 2017
@damienalexandre damienalexandre deleted the addMetaToMessages branch June 1, 2017 09: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