Code review comment for lp:~om26er/ubuntu-system-settings/pep8_pyflakes_fix

Revision history for this message
Omer Akram (om26er) wrote :

> Thanks for the work there, nice job, some comments though:
>
Thanks for the review.

> - why do you drop the use of translations? some of us run non-english locales
> and it's important to be able to run the tests under those
>

In an optimal world we should not be testing labels in our apps because they are well tested in the UIToolkit, but in this case there are many places where we are testing label text which we should not be, My Further branches for system-settings will try and remove the reliability on labels as much as possible.

Instead of translating I am making sure that the app always loads en.US note:

35 + self.patch_environment('LC_MESSAGES', 'en_US.UTF-8')

> - why those sort of changes
>
> 690 - """ Checks whether the Battery plugin is not available as we have no
> battery """
> 691 - self.assertThat(lambda: self.main_view.select_single(objectName
> ='entryComponent-battery'),
> 692 - raises(StateNotFoundError))
> 693 + """ Checks whether the Battery plugin is not available as we
> 694 + have no battery
> 695 +
> 696 + """
>
> the emtpy line at the end of the comment seems weird?
>

Fixed that.

> - could you add a test that runs pep8 and pyflake? so we can make sure we
> don't introduce new issues with futur commits

Added two new tests to test both pyflakes and pep8, they currently are skipped if the related tool is not installed, do you think it makes sense for our suite to depend on pyflakes and pep8 ?

« Back to merge proposal