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. > > Also, "operator not preceded by space" lint issues in this particular > > context seem to be opposed to our style guide. It would be best to > > raise this on launchpad-dev mailing list to come to an agreement and to > > see if we can fix either pylint or style guide. > > I have sent and email to lp-dev ML. Cool. It seems everybody agrees that we should not change the styleguide, but instead not use pylint as is. You can ignore those warnings and revert that bit of my patch. It'd still be very useful to resolve the pyflakes warnings that you are getting, though. > I have filled a bug report for that: > https://bugs.edge.launchpad.net/ubuntu/+source/lazr.restfulclient/+bug/583426 Excellent, thanks a lot. > [snip] > > > > > > > === modified file 'lib/lp/translations/interfaces/potemplate.py' > > > > ... > > > > > source_file = Object( > > > > > title=_('Source file for this translation template'), > > > > > readonly=True, schema=ILibraryFileAlias) > > > > > > > > I don't remember why we decided not to export this one? It could be > > > > useful for things like xpipo conversion (with XPI-based translations, > > > > it should contain a reference to imported en-US.xpi). > > > > > > > > Though, we should probably just export it as the public URL, so not a > > > > big deal (let's not block this branch on this). > > > > > > I will leave it for another branch, but for me, it would make sense if > > > this attribute would be used for all translation templates, not only > > > XPI. > > > > Well, we can reconstruct the gettext POT file completely from what we > > have in the DB. Thus, it's not needed. > > My note was about creating a more consistent API so that in the API > source_file (or whatever name we choose for it) would return the > template file for all formats, not only XPI. Sure... > Instead of exposing source_file, which is used only by XPI, maybe we can > add add a getSourceFile method that will return a librarianfile or > generate the POTemplate on the fly. But "no!" :) generating a potemplate on the fly might take some significant time (like with ddtp-ubuntu templates containing ~60000 messages). It would work fine most of the time, but it would also time out when the load on our servers is too high. It is conceivable that we might be able to do it, but I'd rather not worry about that just yet (and such a method should surely not be publicly accessible). > > > > > === modified file 'lib/lp/translations/interfaces/webservice.py' > > > > > --- lib/lp/translations/interfaces/webservice.py 2009-07-17 > > > > 02:25:09 > > > > +0000 > > > > > +++ lib/lp/translations/interfaces/webservice.py 2010-05-19 > > > > 12:09:43 > > > > +0000 > > > > > @@ -1,9 +1,16 @@ > > > > > # Copyright 2009 Canonical Ltd. This software is licensed under the > > > > > # GNU Affero General Public License version 3 (see the file LICENSE). > > > > > > > > > > +# pylint: disable-msg=W0611 > > > > > + > > > > > """All the interfaces that are exposed through the webservice.""" > > > > > > > > > > from lp.translations.interfaces.translationimportqueue import ( > > > > > IHasTranslationImports, > > > > > ITranslationImportQueue, > > > > > ITranslationImportQueueEntry) > > > > > + > > > > > +from lp.translations.interfaces.potemplate import ( > > > > > + IPOTemplate) > > > > > +from lp.translations.interfaces.pofile import ( > > > > > + IPOFile) > > > > > > > > See comment above on the lint output. > > > > > > I added # pylint: disable-msg=W0611, but I don't know what else can be > > > done to avoid the pyflakes warnings. > > > > 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. > > > === 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. > Below is the latest diff, containing your patch together with __all__ > for webservice.py, but with the spaces around = removed. > > It is strange that even if 'make lint' was cheking > lib/lp/translations/model/potemplate.py, it was not raising the errors > fixed in your patch. Very, very interesting. It seems pyflakes and pylint run slightly differently for us. > === modified file 'lib/lp/translations/interfaces/webservice.py' > --- lib/lp/translations/interfaces/webservice.py 2010-05-18 15:27:05 > +0000 > +++ lib/lp/translations/interfaces/webservice.py 2010-05-20 17:38:50 > +0000 > @@ -5,6 +5,14 @@ > > """All the interfaces that are exposed through the webservice.""" > > +__all__ = [ > + 'IHasTranslationImports', > + 'IPOFile', > + 'IPOTemplate', > + 'ITranslationImportQueue', > + 'ITranslationImportQueueEntry', > + ] > + > from lp.translations.interfaces.translationimportqueue import ( > IHasTranslationImports, > ITranslationImportQueue, > @@ -14,3 +22,11 @@ > IPOTemplate) > from lp.translations.interfaces.pofile import ( > IPOFile) > + > +__all__ = [ > + 'IHasTranslationImports', > + 'IPOFile', > + 'IPOTemplate', > + 'ITranslationImportQueue', > + 'ITranslationImportQueueEntry', > + ] Watch for the __all__ defined twice.