On Thu, 2010-05-20 at 16:33 +0000, Данило Шеган wrote: > Hi Adi, > > Thanks for the updates. There's still just a little bit more to do :) > > У сре, 19. 05 2010. у 20:13 +0000, Adi Roiban пише: > > > There are still some warnings for code like: > > > > @operation_parameters( > > pocket=Choice( > > title=_("Pocket"), required=True, > > vocabulary=DBEnumeratedType)) > > > > and it looks fine to me, but pylint complains about: > > > > lib/lp/registry/interfaces/sourcepackage.py > > 191: [C0322, ISourcePackage.getBranch] Operator not preceded by a > > space > > required=True, > > ^ > > vocabulary=DBEnumeratedType)) > > > > > > > > == Pyflakes notices == > > > > > > > > lib/lp/translations/interfaces/webservice.py > > > > 8: 'IHasTranslationImports' imported but unused > > > > 8: 'ITranslationImportQueue' imported but unused > > > > 8: 'ITranslationImportQueueEntry' imported but unused > > > > 13: 'IPOTemplate' imported but unused > > > > 15: 'IPOFile' imported but unused > > > > > > You should add the following to webservice.py if that's the intention: > > > > > > __all__ = [ > > > 'IHasTranslationImports', > > > 'IPOFile', > > > 'IPOTemplate', > > > 'ITranslationImportQueue', > > > 'ITranslationImportQueueEntry', > > > ] > > > > > > But, why is this necessary? > > > > Only interfaces listed in webservices.py will be exported by > > lazr.restful. > > > > Adding __all__ will still generate Pyflakes notices as they are still > > unused in that file. > > 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 > 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. > > > > warning: Not importing directory '/usr/share/pyshared/lazr': missing > > > __init__.py > > > > warning: Not importing directory '/usr/share/pyshared/lazr': missing > > > __init__.py > > > > warning: Not importing directory '/usr/share/pyshared/lazr': missing > > > __init__.py > > > > > > This might be a bug in lazr that needs fixing. Can you please check and > > > file appropriate bug if it is? > > > > Is is not a problem in lazr. I have the same problem with: > > '/usr/share/pyshared/google': missing __init__.py > > > > Those file were installed as dependencies for ubuntuone-client-gnome by > > python-protobuf and python-lazr-uri, python-lazr-restfulclient and are > > maintained by main Ubuntu developers... not in the LP PPA. > > Well, LAZR is a library shared between Launchpad and UbuntuOne. It's > still a bug that would need to be filed. I've worked-around it locally > by doing "sudo touch /usr/share/pyshared/lazr/__init__.py", and there is > probably no reason why the package shouldn't do the same for everybody. I have filled a bug report for that: https://bugs.edge.launchpad.net/ubuntu/+source/lazr.restfulclient/+bug/583426 [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. 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. > > > > === 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) > > === 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. 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. Cheers, === modified file 'lib/lp/registry/interfaces/distroseries.py' --- lib/lp/registry/interfaces/distroseries.py 2010-05-19 17:04:11 +0000 +++ lib/lp/registry/interfaces/distroseries.py 2010-05-20 17:50:00 +0000 @@ -114,7 +114,7 @@ def _validate(self, version): """See `UniqueField`.""" - super(DistroSeriesVersionField, self)._validate(version) + DistroSeriesVersionField._validate(self, version) if not sane_version(version): raise LaunchpadValidationError( "%s is not a valid version" % version) === 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', + ] === modified file 'lib/lp/translations/model/potemplate.py' --- lib/lp/translations/model/potemplate.py 2010-05-17 10:16:03 +0000 +++ lib/lp/translations/model/potemplate.py 2010-05-20 17:38:50 +0000 @@ -1297,8 +1297,8 @@ elif sourcepackagename is None: # Multiple matches, and for a product not a package. logging.warn( - "Found %d templates with path '%s' for productseries % s" % ( - len(matches), path, productseries.title)) + "Found %d templates with path '%s' for productseries % s", + len(matches), path, productseries.title) return None else: # Multiple matches, for a distribution package. Prefer a @@ -1316,9 +1316,9 @@ else: logging.warn( "Found %d templates with path '%s' for package %s " - "(%d matched on from_sourcepackagename)." % ( - len(matches), path, sourcepackagename.name, - len(preferred_matches))) + "(%d matched on from_sourcepackagename).", + len(matches), path, sourcepackagename.name, + len(preferred_matches)) return None @staticmethod @@ -1510,10 +1510,6 @@ if flag ]) - # Store sequences so we can detect later whether we changed the - # message. - sequence = row.sequence - # Store the message. messages.append(msgset) -- Adi Roiban