Code review comment for lp:~adiroiban/launchpad/bug-201749

Am 12.04.2010 02:39, schrieb Adi Roiban:
> I'm back in business.

Good to hear! ;-)

>
> Here are the changes after applying your JS code and updating the windmill tests.

Thanks for updating those tests. I actually forgot to try my code on plural
forms but I am glad to see that works, too. ;-)

>
> I have also merged with devel.
>
> If everything is ok, can you please send this branch to ec2-test?

I will but I have two little commenting and naming complaints that you should
please take care of first.

>
> Thanks!

You are most welcome! ;-)

Henning

> === modified file 'lib/canonical/launchpad/javascript/translations/pofile.js'
> --- lib/canonical/launchpad/javascript/translations/pofile.js 2010-03-23 00:22:40 +0000
> +++ lib/canonical/launchpad/javascript/translations/pofile.js 2010-04-11 23:07:06 +0000
> @@ -374,10 +374,25 @@
> return;
> }
> if (this.get('checked') == true) {
> + // When checking the "Need review" checkbox the new
> + // translation should be automatically checked.

Please don't use "should" in comments, this is not a test. ;-)

Also, it is not necessary to explain what the code does unless the code is
really complicated or some special condition/work-around/bug etc has to be
known to understand the code.

Generally, the code should speak for itself. If you feel the code does not,
maybe a variable needs a more descriptive name?

> var select = Y.one('#' + translation_id + '_select');
> if (select !== null) {
> select.set('checked', true);
> }
> + } else {
> + // When unchecking the "Need review" checkbox and the new
> + // translation is empty, the current translation should be
> + // automatically selected.

Same here.

> + var inputfield = Y.one('#' + translation_id);
> + if (inputfield !== null&& inputfield.get('value') == '') {
> + var current_radio_id = translation_id.replace(
> + /_new$/, '_radiobutton');
> + var current_radio = Y.one('#' + current_radio_id);
> + if (current_radio !== null) {
> + current_radio.set('checked', true);
> + }
> + }
> }
> },
> node , node, fields[key]
>
> === modified file 'lib/lp/translations/windmill/tests/test_pofile_translate.py'
> --- lib/lp/translations/windmill/tests/test_pofile_translate.py 2010-03-23 00:22:40 +0000
> +++ lib/lp/translations/windmill/tests/test_pofile_translate.py 2010-04-11 23:40:51 +0000
> @@ -52,8 +52,8 @@
> client.asserts.assertChecked(id=radiobutton_id)
>
> def _check_reset_translation_select(

Sorry, I did not notice this before. Methods are always camelCase, functions
and variables use_underscores. It's in the style guide ... ;-)

       def _checkResetTranslationSelect(

That's all.