Launchpad itself

Code review comment for lp:~jtv/launchpad/recife-552639

recife-552639
Merge into recife

Revision history for this message

Henning Eggers (henninge) wrote on 2010-06-21:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 19.06.2010 07:27, schrieb Jeroen T. Vermeulen:
[...]
>>> +=======
>>> + def _nameMessageStatus(self, message, translation_side_traits):
>>> + """Figure out the decision-matrix status of a message.
>>> +
>>> + This is used in navigating the decision matrix in
>>> + `setCurrentTranslation`.
>>> + """
>>> + if message is None:
>>> + return 'none'
>>> + elif message.potemplate is None:
>>> + if translation_side_traits.other_side.getFlag(message):
>>> + return 'other_shared'
>>> + else:
>>> + return 'shared'
>>> + else:
>>> + assert message.potemplate is not None, "Confused message
>> state."
>>
>> Your either forgot this in here or it's paranoia. Since the preceding elif
>> checks the exact same condition, this really is not needed.
>
> It's also partly caused by the "there must be an else" rule. I resolved this by:

Well, the usual way to fulfill that is "comment and pass". But the new 'if'
statement is much better anyway.

>
> 1. Adding an ITranslationMessage.is_diverged to allow for clearer checks.
> 2. Moving the "else" clause above the "elif" clause.
> 3. Flattening the "if" inside the "elif" clause to be part of the main if/elif/else.
>
> Looks much nicer now. We should have added is_diverged earlier.

Yes, definitely! Thanks for finally doing this.

>
>
>>> + def setCurrentTranslation(self, pofile, submitter, translations,
>> origin,
>>> + translation_side,
>> share_with_other_side=False):
>>> + """See `IPOTMsgSet`."""
>>> + traits = make_translation_side_message_traits(
>>> + translation_side, self, pofile.potemplate, pofile.language,
>>> + variant=pofile.variant)
>>> +
>>> + translations = self._findPOTranslations(translations)
>>> + incumbent_message = traits.incumbent_message
>>> + twin = self._findTranslationMessage(
>>> + pofile, translations, prefer_shared=False)
>>
>> Hm, I am not 100% sure what a twin is. Maybe you could explain that here?
>
> I thought the blueprint made that clear, but I've now documented it in the code.

Oh, I guess I was too slow. But "twin" just never appeared before. Thanks for
documenting it now.

>
>
>>> + # Summary of the matrix:
>>> + # * If the incumbent message is diverged and we're setting a
>>> + # translation that's already shared: converge.
>>> + # * If the incumbent message is diverged and we're setting a
>>> + # translation that's not already shared: maintain divergence.
>>> + # * If the incumbent message is shared, replace it.
>>> + # * If there is no twin, simply create a new message (shared or
>>> + # diverged depending; see above).
>>> + # * If there is a shared twin, activate it (but also diverge if
>>> + # necessary; see above).
>>> + # * If there is a diverged twin, activate it (and converge it
>>> + # if appropriate; see above).
>>> + # * If there is a twin that's shared on the other side,
>>> +
>>> + # XXX: Steal flag if policy permits and either:
>>
>> Do we still need to agree on this policy?
>
> No, this comment was temporary and I removed it.

Good

>
>
>>> + # - there is no shared active message on the other side, or
>>> + # - we're returning a shared message.
>>> +
>>> + decision_matrix = {
>>> + 'incumbent_none': {
>>> + 'twin_none': 'Z1*',
>>> + 'twin_shared': 'Z4*',
>>> + 'twin_diverged': 'Z7*',
>>> + 'twin_other_shared': 'Z4',
>>> + },
>>> + 'incumbent_shared': {
>>> + 'twin_none': 'B1*',
>>> + 'twin_shared': 'B4*',
>>> + 'twin_diverged': 'B7*',
>>> + 'twin_other_shared': 'B4',
>>> + },
>>> + 'incumbent_diverged': {
>>> + 'twin_none': 'A2',
>>> + 'twin_shared': 'A5',
>>> + 'twin_diverged': 'A4',
>>> + 'twin_other_shared': 'A6',
>>> + },
>>> + 'incumbent_other_shared': {
>>> + 'twin_none': 'B1+',
>>> + 'twin_shared': 'B4+',
>>> + 'twin_diverged': 'B7+',
>>> + 'twin_other_shared': '',
>>
>> This last one should be '4' instead of special casing it at the end.
>
> That's true, but it's not time for that yet. In the design, this case means "the message we want to activate is already active and nothing needs to be done." It happens that we could implement it in the same way as 4, but that is a detail of my current implementation. We made a conscious decision to implement the decision matrix that we designed in Recife first, faithfully and directly. And then later, when we're confident that the tests represent the exact behaviour we want, we can start refactoring the code.

OK, I understand that now.

>
> What I've been doing about these possible simplifications is document them in the design as optional steps. I've documented this field as (B4+) which means we can choose to implement it as B4+ but it won't do anything.
>
>
>>> + },
>>> + }
>>> +
>>> + incumbent_state = "incumbent_%s" % self._nameMessageStatus(
>>> + incumbent_message, traits)
>>> + twin_state = "twin_%s" % self._nameMessageStatus(twin, traits)
>>> +
>>> + decisions = decision_matrix[incumbent_state][twin_state]
>>> + file('/tmp/matrix.log','a').write(decisions+'\n') # XXX: DEBUG CODE
>>> + assert re.match('[ABZ]?[124567]?[+*]?$', decisions), (
>>> + "Bad decision string.")
>>
>> Paranoia!
>
> I removed the logging code. I kept the regex assertion because it checks the <letter><digit><+*> structure as well as the actual contents of the string. We can get rid of that when we leave the matrix structure behind.

As you wish. ;)

>
>
>>> +
>>> + for character in decisions:
>>> + if character == 'A':
>>> + # Deactivate & converge.
>>> + # There may be an identical shared message.
>>> + traits.setFlag(incumbent_message, False)
>>> + incumbent_message.shareIfPossible()
>>> + elif character == 'B':
>>> + # Deactivate.
>>> + traits.setFlag(incumbent_message, False)
>>> + elif character == 'Z':
>>> + # There is no incumbent message, so do nothing to it.
>>> + assert incumbent_message is None, (
>>> + "Incorrect Z in decision matrix.")
>>
>> Ok, first you hide this check in _nameMessageStatus, now you pull it out in
>> the open for an assert. Maybe you should trust your matrix more and just put a
>> "pass" in here?
>
> Again rather not in this phase, where we want to be sure we can tweak things
without breaking them. Once we start refactoring, the Z probably becomes a
no-op anyway.

Yes, I understand now that this is a multiple-phase project.

>
>
>>> + elif character == '1':
>>> + # Create & activate.
>>> + message = self._makeTranslationMessage(
>>> + pofile, submitter, translations, origin)
>>> + elif character == '2':
>>> + # Create, diverge, activate.
>>> + message = self._makeTranslationMessage(
>>> + pofile, submitter, translations, origin, diverged=True)
>>> + elif character == '4':
>>> + # Activate.
>>> + message = twin
>>> + elif character == '5':
>>> + # If other is a suggestion, diverge and activate.
>>> + # (If not, it's already active and has been unmasked by
>>> + # our deactivating the incumbent).
>>> + message = twin
>>> + if not traits.getFlag(twin):
>>> + assert not traits.other_side.getFlag(twin), (
>>> + "Decision matrix says '5' for a message that's "
>>> + "active on the other side.")
>>
>> Yes, that is what is done in _nameMessageStatus, so duplicated here. Also, the
>> error message is quite cryptic to an outsider.
>
> Alright, I've made the message clearer.

I don't see that in the current diff.

>
>
>>> +
>>> + traits.setFlag(message, True)
>>> +
>>> + return message
>>> +
>>> +>>>>>>> MERGE-SOURCE
>>> def resetCurrentTranslation(self, pofile, lock_timestamp):
>>> """See `IPOTMsgSet`."""
>>>
>>> @@ -1145,7 +1423,7 @@
>>> # The credits message has a fixed "translator."
>>> translator = getUtility(ILaunchpadCelebrities).rosetta_experts
>>>
>>> - message = self.updateTranslation(
>>> + self.updateTranslation(
>>> pofile, translator, [credits_message_str],
>>> is_current_upstream=False, allow_credits=True,
>>> force_shared=True, force_edition_rights=True,
>>> === modified file 'lib/lp/translations/tests/test_potmsgset.py'
>>
>> The tests look very good although I am sure there could be even more.... ;-)
>
> I'm not sure what you mean. As you know (and as mentioned several times in
the initial comments for this MP) there is a separate branch with exhaustive
tests.

Yes, I do know. ;-) Never mind me trying to be funny ...

review approve code

Cheers,
Henning
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkwfQXYACgkQBT3oW1L17ih8wgCgsJG4KDNT99dnZfMtrcMMWn3M
slgAn277jfZvS8SynoxQmJWeCPwl0rkP
=0oh7
-----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 19.06.2010 07:27, schrieb Jeroen T. Vermeulen:
[...]
>>> +=======
>>> +    def _nameMessageStatus(self, message, translation_side_traits):
>>> +        """Figure out the decision-matrix status of a message.
>>> +
>>> +        This is used in navigating the decision matrix in
>>> +        `setCurrentTranslation`.
>>> +        """
>>> +        if message is None:
>>> +            return 'none'
>>> +        elif message.potemplate is None:
>>> +            if translation_side_traits.other_side.getFlag(message):
>>> +                return 'other_shared'
>>> +            else:
>>> +                return 'shared'
>>> +        else:
>>> +            assert message.potemplate is not None, "Confused message
>> state."
>>
>> Your either forgot this in here or it's paranoia. Since the preceding elif
>> checks the exact same condition, this really is not needed.
> 
> It's also partly caused by the "there must be an else" rule.  I resolved this by:

Well, the usual way to fulfill that is "comment and pass". But the new 'if'
statement is much better anyway.

> 
> 1. Adding an ITranslationMessage.is_diverged to allow for clearer checks.
> 2. Moving the "else" clause above the "elif" clause.
> 3. Flattening the "if" inside the "elif" clause to be part of the main if/elif/else.
> 
> Looks much nicer now.  We should have added is_diverged earlier.

Yes, definitely! Thanks for finally doing this.

> 
> 
>>> +    def setCurrentTranslation(self, pofile, submitter, translations,
>> origin,
>>> +                              translation_side,
>> share_with_other_side=False):
>>> +        """See `IPOTMsgSet`."""
>>> +        traits = make_translation_side_message_traits(
>>> +            translation_side, self, pofile.potemplate, pofile.language,
>>> +            variant=pofile.variant)
>>> +
>>> +        translations = self._findPOTranslations(translations)
>>> +        incumbent_message = traits.incumbent_message
>>> +        twin = self._findTranslationMessage(
>>> +            pofile, translations, prefer_shared=False)
>>
>> Hm, I am not 100% sure what a twin is. Maybe you could explain that here?
> 
> I thought the blueprint made that clear, but I've now documented it in the code.

Oh, I guess I was too slow. But "twin" just never appeared before. Thanks for
documenting it now.

> 
> 
>>> +        # Summary of the matrix:
>>> +        #  * If the incumbent message is diverged and we're setting a
>>> +        #    translation that's already shared: converge.
>>> +        #  * If the incumbent message is diverged and we're setting a
>>> +        #    translation that's not already shared: maintain divergence.
>>> +        #  * If the incumbent message is shared, replace it.
>>> +        #  * If there is no twin, simply create a new message (shared or
>>> +        #    diverged depending; see above).
>>> +        #  * If there is a shared twin, activate it (but also diverge if
>>> +        #    necessary; see above).
>>> +        #  * If there is a diverged twin, activate it (and converge it
>>> +        #    if appropriate; see above).
>>> +        #  * If there is a twin that's shared on the other side,
>>> +
>>> +        # XXX: Steal flag if policy permits and either:
>>
>> Do we still need to agree on this policy?
> 
> No, this comment was temporary and I removed it.

Good

> 
> 
>>> +        #  - there is no shared active message on the other side, or
>>> +        #  - we're returning a shared message.
>>> +
>>> +        decision_matrix = {
>>> +            'incumbent_none': {
>>> +                'twin_none': 'Z1*',
>>> +                'twin_shared': 'Z4*',
>>> +                'twin_diverged': 'Z7*',
>>> +                'twin_other_shared': 'Z4',
>>> +            },
>>> +            'incumbent_shared': {
>>> +                'twin_none': 'B1*',
>>> +                'twin_shared': 'B4*',
>>> +                'twin_diverged': 'B7*',
>>> +                'twin_other_shared': 'B4',
>>> +            },
>>> +            'incumbent_diverged': {
>>> +                'twin_none': 'A2',
>>> +                'twin_shared': 'A5',
>>> +                'twin_diverged': 'A4',
>>> +                'twin_other_shared': 'A6',
>>> +            },
>>> +            'incumbent_other_shared': {
>>> +                'twin_none': 'B1+',
>>> +                'twin_shared': 'B4+',
>>> +                'twin_diverged': 'B7+',
>>> +                'twin_other_shared': '',
>>
>> This last one should be '4' instead of special casing it at the end.
> 
> That's true, but it's not time for that yet.  In the design, this case means "the message we want to activate is already active and nothing needs to be done."  It happens that we could implement it in the same way as 4, but that is a detail of my current implementation.  We made a conscious decision to implement the decision matrix that we designed in Recife first, faithfully and directly.  And then later, when we're confident that the tests represent the exact behaviour we want, we can start refactoring the code.

OK, I understand that now.

> 
> What I've been doing about these possible simplifications is document them in the design as optional steps.  I've documented this field as (B4+) which means we can choose to implement it as B4+ but it won't do anything.
> 
> 
>>> +            },
>>> +        }
>>> +
>>> +        incumbent_state = "incumbent_%s" % self._nameMessageStatus(
>>> +            incumbent_message, traits)
>>> +        twin_state = "twin_%s" % self._nameMessageStatus(twin, traits)
>>> +
>>> +        decisions = decision_matrix[incumbent_state][twin_state]
>>> +        file('/tmp/matrix.log','a').write(decisions+'\n') # XXX: DEBUG CODE
>>> +        assert re.match('[ABZ]?[124567]?[+*]?$', decisions), (
>>> +            "Bad decision string.")
>>
>> Paranoia!
> 
> I removed the logging code.  I kept the regex assertion because it checks the <letter><digit><+*> structure as well as the actual contents of the string.  We can get rid of that when we leave the matrix structure behind.

As you wish. ;)

> 
> 
>>> +
>>> +        for character in decisions:
>>> +            if character == 'A':
>>> +                # Deactivate & converge.
>>> +                # There may be an identical shared message.
>>> +                traits.setFlag(incumbent_message, False)
>>> +                incumbent_message.shareIfPossible()
>>> +            elif character == 'B':
>>> +                # Deactivate.
>>> +                traits.setFlag(incumbent_message, False)
>>> +            elif character == 'Z':
>>> +                # There is no incumbent message, so do nothing to it.
>>> +                assert incumbent_message is None, (
>>> +                    "Incorrect Z in decision matrix.")
>>
>> Ok, first you hide this check in _nameMessageStatus, now you pull it out in
>> the open for an assert. Maybe you should trust your matrix more and just put a
>> "pass" in here?
>
> Again rather not in this phase, where we want to be sure we can tweak things
without breaking them.  Once we start refactoring, the Z probably becomes a
no-op anyway.

Yes, I understand now that this is a multiple-phase project.

>
>
>>> +            elif character == '1':
>>> +                # Create & activate.
>>> +                message = self._makeTranslationMessage(
>>> +                    pofile, submitter, translations, origin)
>>> +            elif character == '2':
>>> +                # Create, diverge, activate.
>>> +                message = self._makeTranslationMessage(
>>> +                    pofile, submitter, translations, origin, diverged=True)
>>> +            elif character == '4':
>>> +                # Activate.
>>> +                message = twin
>>> +            elif character == '5':
>>> +                # If other is a suggestion, diverge and activate.
>>> +                # (If not, it's already active and has been unmasked by
>>> +                # our deactivating the incumbent).
>>> +                message = twin
>>> +                if not traits.getFlag(twin):
>>> +                    assert not traits.other_side.getFlag(twin), (
>>> +                        "Decision matrix says '5' for a message that's "
>>> +                        "active on the other side.")
>>
>> Yes, that is what is done in _nameMessageStatus, so duplicated here. Also, the
>> error message is quite cryptic to an outsider.
>
> Alright, I've made the message clearer.

I don't see that in the current diff.

>
>
>>> +
>>> +        traits.setFlag(message, True)
>>> +
>>> +        return message
>>> +
>>> +>>>>>>> MERGE-SOURCE
>>>      def resetCurrentTranslation(self, pofile, lock_timestamp):
>>>          """See `IPOTMsgSet`."""
>>>
>>> @@ -1145,7 +1423,7 @@
>>>          # The credits message has a fixed "translator."
>>>          translator = getUtility(ILaunchpadCelebrities).rosetta_experts
>>>
>>> -        message = self.updateTranslation(
>>> +        self.updateTranslation(
>>>              pofile, translator, [credits_message_str],
>>>              is_current_upstream=False, allow_credits=True,
>>>              force_shared=True, force_edition_rights=True,
>>> === modified file 'lib/lp/translations/tests/test_potmsgset.py'
>>
>> The tests look very good although I am sure there could be even more.... ;-)
>
> I'm not sure what you mean.  As you know (and as mentioned several times in
the initial comments for this MP) there is a separate branch with exhaustive
tests.

Yes, I do know. ;-) Never mind me trying to be funny ...

review approve code

Cheers,
Henning
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkwfQXYACgkQBT3oW1L17ih8wgCgsJG4KDNT99dnZfMtrcMMWn3M
slgAn277jfZvS8SynoxQmJWeCPwl0rkP
=0oh7
-----END PGP SIGNATURE-----

review: Approve (code)

« Back to merge proposal