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

On Thu, 2010-05-20 at 18:48 +0000, Данило Шеган wrote:
> Hey Adi, we are getting very close. Just a few more small issues to
> fix.
>
> У чет, 20. 05 2010. у 18:00 +0000, Adi Roiban пише:
>
> > > It actually fixed it for me. I'm attaching a patch which fixes all the
> > > lint issues I've seen. It would be nicer if you did that instead :)
> >
> > I have no idea why this is not fixing for me.
> >
> > Here is the output for `make lint` after applying your patch:
> >
> > = Launchpad lint =
> >
> > Checking for conflicts. and issues in doctests and templates.
> > Running jslint, xmllint, pyflakes, and pylint.
> > Using normal rules.
> >
> > Linting changed files:
> > lib/lp/registry/interfaces/distroseries.py
> > lib/lp/registry/interfaces/sourcepackage.py
> > lib/lp/translations/interfaces/webservice.py
> > lib/lp/translations/model/potemplate.py
> >
> > == Pyflakes notices ==
> >
> > lib/lp/translations/interfaces/webservice.py
> > 16: 'IHasTranslationImports' imported but unused
> > 16: 'ITranslationImportQueue' imported but unused
> > 16: 'ITranslationImportQueueEntry' imported but unused
> > 21: 'IPOTemplate' imported but unused
> > 23: 'IPOFile' imported but unused
>
> Hum, very interesting. Perhaps it's a problem with pyflakes
> incompatibilities? (FWIW, it seems your branch now specifies __all__
> twice)
>
> FWIW, defining __all__ fixes pyflakes warnings for me, but not the
> pylint warnings.

I have removed the duplicate __all__ and only leave it before the
imports.

[snip]
> > > Add __all__: have you tried it at all? It totally works for me, and I'd
> > > be very surprised if it doesn't work for you. Also, __all__ is required
> > > in all our modules as well (importfascist used to check that before,
> > > maybe it doesn't anymore)
>
> I'd really like to get this resolved. It seems the line length is also
> slightly differently defined for you:
>
> === modified file 'lib/lp/translations/interfaces/pofile.py'
> --- lib/lp/translations/interfaces/pofile.py 2010-05-19 17:04:11 +0000
> +++ lib/lp/translations/interfaces/pofile.py 2010-05-20 17:58:16 +0000
> @@ -127,9 +127,9 @@
> '''),
> required=False)
>
> - translation_messages = Attribute(_('''
> - All `ITranslationMessage` objects related to this translation file.
> - '''))
> + translation_messages = Attribute(_(
> + "All `ITranslationMessage` objects related to this "
> + "translation file."))
>
> plural_forms = Int(
> title=_('Number of plural forms for the language of this PO file.'),
>
>
> This is not needed for me, because just putting it as
>
> translation_messages = Attribute(_(
> 'All `ITranslationMessage` objects related to this translation file.'
> ))
>
> works fine. Not that there's anything wrong with your approach, I am just noting this.

This will break the styleguide as there is a space before the ')'

> > > > === modified file 'lib/lp/translations/interfaces/pofile.py'
> > > > --- lib/lp/translations/interfaces/pofile.py 2010-03-08 21:06:34 +0000
> > > > +++ lib/lp/translations/interfaces/pofile.py 2010-05-19 17:04:11 +0000
> > > > @@ -127,9 +127,9 @@
> > > > '''),
> > > > required=False)
> > > >
> > > > - translation_messages = Attribute(_(
> > > > - 'All `ITranslationMessage` objects related to this translation
> > > > file.'
> > > > - ))
> > > > + translation_messages = Attribute(_('''
> > > > + All `ITranslationMessage` objects related to this translation
> > > > file.
> > > > + '''))
> > >
> > > Was this change really required? It makes the description contain a
> > > bunch of spaces and newlines instead, which is not really desired.
> >
> > :) Not really, but the previous code was not following the styleguide as
> > there was a space before ')'.
> >
> > I have used another solution.
>
> What is the "another solution" you used? I see that you didn't really
> change anything, which is fine, since pylint seems to be useless.

This was my "other solution".

- translation_messages = Attribute(_('''
- All `ITranslationMessage` objects related to this translation file.
- '''))
+ translation_messages = Attribute(_(
+ "All `ITranslationMessage` objects related to this "
+ "translation file."))

[snip]
> > +
> > +__all__ = [
> > + 'IHasTranslationImports',
> > + 'IPOFile',
> > + 'IPOTemplate',
> > + 'ITranslationImportQueue',
> > + 'ITranslationImportQueueEntry',
> > + ]
>
> Watch for the __all__ defined twice.
Fixed.

Thanks for the review!
Here is the latest diff.

=== modified file 'lib/lp/translations/interfaces/webservice.py'
--- lib/lp/translations/interfaces/webservice.py 2010-05-20 17:58:16
+0000
+++ lib/lp/translations/interfaces/webservice.py 2010-05-20 19:03:10
+0000
@@ -22,11 +22,3 @@
     IPOTemplate)
 from lp.translations.interfaces.pofile import (
     IPOFile)
-
-__all__ = [
- 'IHasTranslationImports',
- 'IPOFile',
- 'IPOTemplate',
- 'ITranslationImportQueue',
- 'ITranslationImportQueueEntry',
- ]

--
Adi Roiban