[translation] Deprecated DiffOperation #15562

zerustech · 2015-08-16T02:30:30Z

Summary:

The DiffOperation class has been deprecated and TargetOperation
should be used instead, because DiffOperation has nothing to do
with 'diff', thus its class name is misleading.

Also added detailed documents for all operation interface and classes.

Background:

The following names should have consistent meanings for all operations:

The name of intersection is temporarily introduced here to explain this issue.

intersection = source ∩ target = {x: x ∈ source ∧ x ∈ target}
all = result of the operation, depends on the operation.
new = all ∖ source = {x: x ∈ all ∧ x ∉ source}
obsolete = source ∖ all = {x: x ∈ source ∧ x ∉ all}

The following analysis explains why DiffOperation should be deprecated.

Logic of `MergeOperation`:

all = source ∪ target = {x: x ∈ source ∨ x ∈ target}
new = all ∖ source = {x: x ∈ target ∧ ∉ source}
obsolete = source ∖ all = {x: x ∈ source ∧ x ∉ source ∧ x ∉ target} = ∅

This absolutely makes sense.

Logic of `DiffOperation`:

all = intersection ∪ (target ∖ intersection) = target
new = all ∖ source = {x: x ∈ target ∧ x ∉ source}
obsolete = source ∖ all = source ∖ target = {x: x ∈ source ∧ x ∉ target}

The all part is confusing because 'diff' should either mean 'relative complement' or 'symmetric difference' operation:

Relative Complement:

all = source ∖ target = {x: x ∈ source ∧ x ∉ target}

Symmetric Difference:

all = (source ∖ target) ∪ (target ∖ source) = {x: x ∈ source ∧ x ∉ target ∨ x ∈ target ∧ x ∉ source}

Current Logic has Nothing to do with "Diff":

all = intersection ∪ (target ∖ intersection) = target

So the name of DiffOperation is misleading and inappropriate.
Unfortunately, there is no corresponding set operation for this class,
so it's hard to give it an apppriate name.
From my point of view, I believe the most accurate name for this class
should be TargetOperation because its result is same as the target set.

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	n/a

zerustech · 2015-08-16T02:32:24Z

@stof @aitboudad Please check, thanks.

OskarStark · 2015-08-21T17:48:59Z

src/Symfony/Component/Translation/Catalogue/TargetOperation.php

+ * obsolete = source ∖ all = source ∖ target = {x: x ∈ source ∧ x ∉ target}
+ * Basically, the result contains messages from the target catalogue. 
+ *
+ * @author Jean-François Simon <contact@jfsimon.fr>


if you create this class and it is new, is it correct to copy the author?

YEs, author indicates who created the code. This way, you know who to ask for if you're really stuck with a bit of code. That's why I propose to remvoe some of the added @author tags (for instance, from the deprecated class), as the current PR doesn't add much to it.

@OskarStark I copied the author tags because I didn't change the code at all, I just copied the original code from the deprecated class.

@wouterj I added myself as an additional author because I added detailed comments for these classes. They were a bit confusing before and it takes time to figure out the correct meanings of the classes as well as their properties.

@zerustech @author is mostly useful for the author of the code, as explained by @wouterj. Adding only comments does not really need to be added as @author (these annotations are not a way to track all contributors to the class. This is what the git history is for)

@stof I've updated the @author tags

aitboudad · 2015-08-25T12:41:17Z

could you update https://github.com/symfony/symfony/blob/2.8/src/Symfony/Bundle/FrameworkBundle/Command/TranslationUpdateCommand.php#L142 too

zerustech · 2015-08-25T12:48:40Z

@aitboudad Shall I create a separate PR for it after this PR is merged or just include the change in this PR?

aitboudad · 2015-08-25T12:50:20Z

just include the change

zerustech · 2015-08-25T14:32:21Z

@aitboudad Updated

aitboudad · 2015-08-25T16:17:31Z

👍

aitboudad · 2015-08-25T16:17:43Z

ping @symfony/deciders

xabbuh · 2015-08-31T20:33:37Z

src/Symfony/Component/Translation/Tests/Catalogue/DiffOperationTest.php

@@ -15,6 +15,9 @@
 use Symfony\Component\Translation\MessageCatalogue;
 use Symfony\Component\Translation\MessageCatalogueInterface;

+/**
+ * @group legacy
+ */
 class DiffOperationTest extends AbstractOperationTest


What about extending the TargetOperationTest class instead and overriding only the createOperation method instead?

zerustech · 2015-09-01T03:25:12Z

@xabbuh @stof Updated

xabbuh · 2015-09-01T06:48:14Z

Looks like you need to bump the required version of the Translation component in the FrameworkBundle.

zerustech · 2015-09-01T07:07:10Z

@xabbuh Updated

aitboudad · 2015-09-02T16:10:50Z

src/Symfony/Bundle/FrameworkBundle/Tests/Command/TranslationUpdateCommandTest.php

+use Symfony\Bundle\FrameworkBundle\Command\TranslationUpdateCommand;
+use Symfony\Component\Filesystem\Filesystem;
+
+class TranslationUpdateCommandTest extends \PHPUnit_Framework_TestCase


I think this should be added in 2.3

Testing the command is indeed a different topic and should go in a separate PR in 2.3

@aitboudad @stof I've created PR #15673 as per your suggestion. However, if test script is added in a separate PR, how to prove the issue (and avoiding regressions) in this PR? And why should it be added in 2.3?

@zerustech because testing the command makes sense in all versions of Symfony, and so it should be added first in 2.3 (which will then be merged into newer branches)

@stof I agree with you, but why other test scripts like TranslationDebugCommandTest.php was not added in 2.3?

because the TranslationDebugCommand does not exist in 2.3, and so cannot be tested there...

@stof So just to clarify the workflow for adding a new test script:

Find the oldest supported branch that contains the file to be tested.
In this case, the branch is 2.3, but in the future if 2.3 is not supported any more,
the branch might be 2.7 for example.

Add the test script in that branch

Is that correct?

@stof Thanks, I will follow it in the future.

xabbuh · 2015-09-02T18:31:57Z

src/Symfony/Component/Translation/Tests/Catalogue/TargetOperationTest.php

+use Symfony\Component\Translation\Catalogue\TargetOperation;
+use Symfony\Component\Translation\MessageCatalogueInterface;
+
+class TargetOperationTest extends DiffOperationTest


Inheritance should be the other way around. Otherwise, the new TargetOperationTest is a legacy test too.

@xabbuh Fixed

The ``DiffOperation`` class has been deprecated and ``TargetOperation`` should be used instead, because ``DiffOperation`` has nothing to do with 'diff', thus its class name is misleading. Also added detailed documents for all operation interface and classes. The following names should have consistent meanings for all operations: The name of ``intersection`` is temporarily introduced here to explain this issue. * [x] ``intersection`` = source ∩ target = {x: x ∈ source ∧ x ∈ target} * [x] ``all`` = **result of the operation, depends on the operation.** * [x] ``new`` = all ∖ source = {x: x ∈ all ∧ x ∉ source} * [x] ``obsolete`` = source ∖ all = {x: x ∈ source ∧ x ∉ all} The following analysis explains why ``DiffOperation`` should be deprecated. * [x] ``all`` = source ∪ target = {x: x ∈ source ∨ x ∈ target} * [x] ``new`` = all ∖ source = {x: x ∈ target ∧ ∉ source} * [x] ``obsolete`` = source ∖ all = {x: x ∈ source ∧ x ∉ source ∧ x ∉ target} = ∅ This absolutely makes sense. * [ ] ``all`` = intersection ∪ (target ∖ intersection) = target * [x] ``new`` = all ∖ source = {x: x ∈ target ∧ x ∉ source} * [x] ``obsolete`` = source ∖ all = source ∖ target = {x: x ∈ source ∧ x ∉ target} The ``all`` part is confusing because 'diff' should either mean 'relative complement' or 'symmetric difference' operation: * ``all`` = source ∖ target = {x: x ∈ source ∧ x ∉ target} * ``all`` = (source ∖ target) ∪ (target ∖ source) = {x: x ∈ source ∧ x ∉ target ∨ x ∈ target ∧ x ∉ source} * ``all`` = intersection ∪ (target ∖ intersection) = target So the name of ``DiffOperation`` is misleading and inappropriate. Unfortunately, there is no corresponding set operation for this class, so it's hard to give it an apppriate name. From my point of view, I believe the most accurate name for this class should be ``TargetOperation`` because its result is same as the target set. | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a

Added the test script as per the discussion in PR symfony#15562 | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a

xabbuh · 2015-09-05T11:19:14Z

👍

aitboudad · 2015-09-05T11:38:00Z

Thank you @zerustech.

This PR was merged into the 2.8 branch. Discussion ---------- [translation] Deprecated DiffOperation ## Summary: The ``DiffOperation`` class has been deprecated and ``TargetOperation`` should be used instead, because ``DiffOperation`` has nothing to do with 'diff', thus its class name is misleading. Also added detailed documents for all operation interface and classes. ## Background: The following names should have consistent meanings for all operations: The name of ``intersection`` is temporarily introduced here to explain this issue. * [x] ``intersection`` = source ∩ target = {x: x ∈ source ∧ x ∈ target} * [x] ``all`` = **result of the operation, depends on the operation.** * [x] ``new`` = all ∖ source = {x: x ∈ all ∧ x ∉ source} * [x] ``obsolete`` = source ∖ all = {x: x ∈ source ∧ x ∉ all} The following analysis explains why ``DiffOperation`` should be deprecated. ## Logic of ``MergeOperation``: * [x] ``all`` = source ∪ target = {x: x ∈ source ∨ x ∈ target} * [x] ``new`` = all ∖ source = {x: x ∈ target ∧ ∉ source} * [x] ``obsolete`` = source ∖ all = {x: x ∈ source ∧ x ∉ source ∧ x ∉ target} = ∅ This absolutely makes sense. ## Logic of ``DiffOperation``: * [ ] ``all`` = intersection ∪ (target ∖ intersection) = target * [x] ``new`` = all ∖ source = {x: x ∈ target ∧ x ∉ source} * [x] ``obsolete`` = source ∖ all = source ∖ target = {x: x ∈ source ∧ x ∉ target} The ``all`` part is confusing because 'diff' should either mean 'relative complement' or 'symmetric difference' operation: ### Relative Complement: * ``all`` = source ∖ target = {x: x ∈ source ∧ x ∉ target} ### Symmetric Difference: * ``all`` = (source ∖ target) ∪ (target ∖ source) = {x: x ∈ source ∧ x ∉ target ∨ x ∈ target ∧ x ∉ source} ### Current Logic has Nothing to do with "Diff": * ``all`` = intersection ∪ (target ∖ intersection) = target So the name of ``DiffOperation`` is misleading and inappropriate. Unfortunately, there is no corresponding set operation for this class, so it's hard to give it an apppriate name. From my point of view, I believe the most accurate name for this class should be ``TargetOperation`` because its result is same as the target set. | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Commits ------- 353c94d [translation][framework-bundle] Deprecated DiffOperation

zerustech · 2015-09-06T01:37:47Z

@aitboudad No problem.

… (zerustech) This PR was merged into the 2.3 branch. Discussion ---------- [framework-bundle] Add Test for TranslationUpdateCommand Added the test script as per the discussion in PR #15562 | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Commits ------- 232f6fd [framework-bundle] Add Test for TranslationUpdateCommand

carsonbot added the Status: Needs Review label Aug 16, 2015

zerustech force-pushed the translation-catalogue-operation branch 5 times, most recently from dd0908f to ba1311a Compare August 17, 2015 14:17

OskarStark reviewed Aug 21, 2015
View reviewed changes

zerustech force-pushed the translation-catalogue-operation branch from ba1311a to 5a1755e Compare August 25, 2015 12:27

zerustech force-pushed the translation-catalogue-operation branch from 5a1755e to 316277e Compare August 25, 2015 14:30

aitboudad added the Translation label Aug 25, 2015

xabbuh reviewed Aug 31, 2015
View reviewed changes

zerustech force-pushed the translation-catalogue-operation branch from 316277e to 5d59c8e Compare September 1, 2015 03:24

zerustech force-pushed the translation-catalogue-operation branch from 5d59c8e to 4f65a86 Compare September 1, 2015 03:26

zerustech force-pushed the translation-catalogue-operation branch from 4f65a86 to 966b455 Compare September 1, 2015 07:06

aitboudad added Ready Status: Reviewed and removed Status: Needs Review labels Sep 1, 2015

aitboudad reviewed Sep 2, 2015
View reviewed changes

aitboudad removed the Ready label Sep 2, 2015

xabbuh reviewed Sep 2, 2015
View reviewed changes

zerustech force-pushed the translation-catalogue-operation branch from 966b455 to 5ef5a05 Compare September 3, 2015 06:38

zerustech force-pushed the translation-catalogue-operation branch from 5ef5a05 to 353c94d Compare September 3, 2015 06:50

zerustech mentioned this pull request Sep 3, 2015

[framework-bundle] Add Test for TranslationUpdateCommand #15673

Merged

aitboudad added the Ready label Sep 3, 2015

aitboudad merged commit 353c94d into symfony:2.8 Sep 5, 2015

fabpot mentioned this pull request Nov 16, 2015

Release v2.8.0-BETA1 #16550

Merged

Uh oh!

[translation] Deprecated DiffOperation #15562

[translation] Deprecated DiffOperation #15562

Uh oh!

Conversation

zerustech commented Aug 16, 2015

Summary:

Background:

Logic of MergeOperation:

Logic of DiffOperation:

Relative Complement:

Symmetric Difference:

Current Logic has Nothing to do with "Diff":

Uh oh!

zerustech commented Aug 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aitboudad commented Aug 25, 2015

Uh oh!

zerustech commented Aug 25, 2015

Uh oh!

aitboudad commented Aug 25, 2015

Uh oh!

zerustech commented Aug 25, 2015

Uh oh!

aitboudad commented Aug 25, 2015

Uh oh!

aitboudad commented Aug 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zerustech commented Sep 1, 2015

Uh oh!

xabbuh commented Sep 1, 2015

Uh oh!

zerustech commented Sep 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Sep 5, 2015

Uh oh!

aitboudad commented Sep 5, 2015

Uh oh!

zerustech commented Sep 6, 2015

Uh oh!

Uh oh!

Logic of `MergeOperation`:

Logic of `DiffOperation`: