Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[FrameworkBundle] Allow to specify a domain when updating translations#19325

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromantograssiot:console-translation-domain-choice
Jul 18, 2016
Merged

[FrameworkBundle] Allow to specify a domain when updating translations#19325

fabpot merged 1 commit intosymfony:masterfromantograssiot:console-translation-domain-choice
Jul 18, 2016

Conversation

@antograssiot
Copy link
Contributor

@antograssiotantograssiot commentedJul 9, 2016•
edited
Loading

QA
Branch?"master"
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes/
Fixed tickets
LicenseMIT
Doc PRno

The MR add the--domain option to thetranslation:update console command
When working with large number of domains, this helps to reduce the noise in the diff when updating only a translation file.

}

$writer->writeTranslations($operation->getResult(),$input->getOption('output-format'),array('path' =>$bundleTransPath,'default_locale' =>$this->getContainer()->getParameter('kernel.default_locale')));
$writer->writeTranslations($operation->getResult($input->getOption('domain')),$input->getOption('output-format'),array('path' =>$bundleTransPath,'default_locale' =>$this->getContainer()->getParameter('kernel.default_locale')));

Choose a reason for hiding this comment

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

but getResult takes no arguments, or did I miss something?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@nicolas-grekas I added an optional one in src/Symfony/Component/Translation/Catalogue/AbstractOperation.php

@antograssiot
Copy link
ContributorAuthor

ping@nicolas-grekas I've updated as requested.

* {@inheritdoc}
*/
publicfunctiongetResult()
publicfunctiongetResult($domain =null)
Copy link
Member

@nicolas-grekasnicolas-grekasJul 17, 2016•
edited
Loading

Choose a reason for hiding this comment

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

I'm not comfortable with this change. The $operation should already have filtered domains.
I see two alternatives: addsetDomains() onAbstractOperation, or add a$domains argument toMessageCatalogue to make it filter messages it can contain. A third alt. would be to filter at the loader/extractor level, but they look too specific for this. WDYT?

Choose a reason for hiding this comment

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

Or fourth way, maybe the less intrusive one: filter the result itself?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I went with adding setDomains(). but I can still try to update if you want.
Also do you want me to squash the 2 commits ?

@antograssiot
Copy link
ContributorAuthor

@nicolas-grekas the tests are passing locally but fails with low deps on travis due to composer issue. This doesn't seems related to my change, am I right ?


// Exit if no messages found.
if (!count($operation->getDomains())) {
if (!count($domains)) {

Choose a reason for hiding this comment

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

Change not needed anymore, same below

Copy link
ContributorAuthor

@antograssiotantograssiotJul 17, 2016•
edited
Loading

Choose a reason for hiding this comment

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

that was still to avoid some function call but I can remove it

@nicolas-grekas
Copy link
Member

FrameworkBundle now needs Translation 3.2, that's why lowest tests are failing

$domain =$input->getOption('domain');

if (null !==$domain) {
$domains =array_intersect($domains,array($domain));

Choose a reason for hiding this comment

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

I don't think we need this line: when the option is set, let's call setDomains with it

Choose a reason for hiding this comment

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

I may be wrong though here :)

Copy link
ContributorAuthor

@antograssiotantograssiotJul 17, 2016•
edited
Loading

Choose a reason for hiding this comment

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

I'll leave thearray_intersect() because if you use for example--domain nonexisting
Then you'll get an empty array when callinggetDomains() and the command will exit (L155-158)

@antograssiot
Copy link
ContributorAuthor

@nicolas-grekas thanks for your explanation on the Travis build and your review.
It should now be limited to the minimum changes.

@nicolas-grekasnicolas-grekas changed the title[Console] [FrameworkBundle] Allow to specify a domain when updating translation[FrameworkBundle] Allow to specify a domain when updating translationsJul 17, 2016
@nicolas-grekas
Copy link
Member

Looks like your target can be achieved with changing the translation component:
master...nicolas-grekas:console-translation-domain-choice
Or do you miss a test case?

@antograssiot
Copy link
ContributorAuthor

Nicolas I'm sorry I don't get your comment. You tm branch is not up to date with mine but I tested it again in an app and it seems to work as expected.

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedJul 17, 2016•
edited
Loading

@antograssiot if you're fine with my patch, then make it yours. If we don't need to change anything in the translation component, it's better.

@antograssiot
Copy link
ContributorAuthor

antograssiot commentedJul 17, 2016•
edited
Loading

@nicolas-grekas I now understood your first comment. Yes I'm missing a test case.
Your patch will only work when using the--dump-messages flag but when using--force all files will still get generated, that's how I came to modify theAbstractOperation.

A test proving that only one file is generated is missing. I tried to wrote one but I failed and I couldn't find any existing test that could guide me.$this->translationDir.'/Resources/translations seems to always stay empty.

The only solution that I can see without updating the translation component would be to find a way to filter$operation->getResult() froTranslationUpdateCommand.phpbut I don't know how.

@nicolas-grekas
Copy link
Member

@antograssiot
Copy link
ContributorAuthor

@nicolas-grekas It works fine in my case but I'm concerned aboutMessageCatalogue::addMetadata() being a private function.
Thanks for your work on patching this anyway, should I close my PR ?

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedJul 18, 2016•
edited
Loading

I didn't spot the method was private, fixed. No need to close this PR, just make my patch yours by doing e.g.:

git checkout console-translation-domain-choicegit fetch https://github.com/nicolas-grekas/symfony.git console-translation-domain-choicegit reset --hard FETCH_HEADgit push -f origin console-translation-domain-choice

@antograssiot
Copy link
ContributorAuthor

PR updated. Thank you very much@nicolas-grekas for your help and patience

@nicolas-grekas
Copy link
Member

👍

@fabpot
Copy link
Member

Thank you@antograssiot.

@fabpotfabpot merged commita8f3a93 intosymfony:masterJul 18, 2016
fabpot added a commit that referenced this pull requestJul 18, 2016
…ing translations (antograssiot)This PR was merged into the 3.2-dev branch.Discussion----------[FrameworkBundle] Allow to specify a domain when updating translations| Q             | A| ------------- | ---| Branch?       | "master"| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes/| Fixed tickets || License       | MIT| Doc PR        | noThe MR add the `--domain` option to the `translation:update` console commandWhen working with large number of domains, this helps to reduce the noise in the diff when updating only a translation file.Commits-------a8f3a93 [FrameworkBundle] Allow to specify a domain when updating translations
nicolas-grekas added a commit that referenced this pull requestSep 13, 2016
… a merge (jakzal)This PR was merged into the 3.2-dev branch.Discussion----------[FrameworkBundle] Fix TranslationUpdateCommand tests after a merge| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#19921| License       | MIT| Doc PR        | -#19325 relied on a wrong count of messages, which was fixed by#19878.Additionally, the `getContainer()` method on the master branch expect messages to be indexed by domain.Commits-------d093c40 [FrameworkBundle] Fix TranslationUpdateCommand tests after a merge
@fabpotfabpot mentioned this pull requestOct 27, 2016
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@antograssiot@nicolas-grekas@fabpot@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp