Launchpad itself

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

bug-525371
Merge into devel

Revision history for this message

Данило Шеган (danilo) wrote on 2010-05-20:

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 :)

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.

> > > 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.

> Should POTemplate and POFile API be available only to authenticated
> users?

Not, it's fine if they are anonymously available.

> > === modified file
> > 'lib/lp/registry/stories/webservice/xx-distroseries.txt'
> > === modified file
> > 'lib/lp/registry/stories/webservice/xx-project-registry.txt'
> > === modified file
> > 'lib/lp/registry/stories/webservice/xx-source-package.txt'
> >
> > It would be nice if we could unify these. And, I actually believe these
> > additions should be put into lib/lp/translations/stories/webservice
> > test instead: they are testing the IHasTranslationTemplates, not the
> > above objects.
>
> I moved them into
> lib/lp/translations/stories/webservice/xx-potemplate.txt

Thanks.

> > > === 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.

> > > === 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.

Rest of it is good.

Cheers,
Danilo

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 :)

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.

> > > 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.

> Should POTemplate and POFile API be available only to authenticated
> users?

Not, it's fine if they are anonymously available.

> > === modified file
> > 'lib/lp/registry/stories/webservice/xx-distroseries.txt'
> > === modified file
> > 'lib/lp/registry/stories/webservice/xx-project-registry.txt'
> > === modified file
> > 'lib/lp/registry/stories/webservice/xx-source-package.txt'
> > 
> > It would be nice if we could unify these.  And, I actually believe these
> > additions should be put into lib/lp/translations/stories/webservice
> > test instead: they are testing the IHasTranslationTemplates, not the
> > above objects.
> 
> I moved them into
> lib/lp/translations/stories/webservice/xx-potemplate.txt

Thanks.

> > > === 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.

> > > === 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.

Rest of it is good.

Cheers,
Danilo

fix-lint-warnings-for-adi.patch

« Back to merge proposal

 === 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 16:10:38 +0000
@@ -114,7 +114,7 @@ class DistroSeriesVersionField(UniqueFie
      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)
@@ -423,38 +423,38 @@ class IDistroSeriesPublic(
      @operation_parameters(
          created_since_date=Datetime(
--            title=_("Created Since Timestamp"),
--            description=_(
++            title = _("Created Since Timestamp"),
++            description = _(
                  "Return items that are more recent than this timestamp."),
--            required=False),
++            required = False),
          status=Choice(
              # Really PackageUploadCustomFormat, patched in
              # _schema_circular_imports.py
--            vocabulary=DBEnumeratedType,
--            title=_("Package Upload Status"),
--            description=_("Return only items that have this status."),
--            required=False),
++            vocabulary = DBEnumeratedType,
++            title = _("Package Upload Status"),
++            description = _("Return only items that have this status."),
++            required = False),
          archive=Reference(
              # Really IArchive, patched in _schema_circular_imports.py
--            schema=Interface,
--            title=_("Archive"),
--            description=_("Return only items for this archive."),
--            required=False),
++            schema = Interface,
++            title = _("Archive"),
++            description = _("Return only items for this archive."),
++            required = False),
          pocket=Choice(
              # Really PackagePublishingPocket, patched in
              # _schema_circular_imports.py
--            vocabulary=DBEnumeratedType,
--            title=_("Pocket"),
--            description=_("Return only items targeted to this pocket"),
--            required=False),
++            vocabulary = DBEnumeratedType,
++            title = _("Pocket"),
++            description = _("Return only items targeted to this pocket"),
++            required = False),
          custom_type=Choice(
              # Really PackageUploadCustomFormat, patched in
              # _schema_circular_imports.py
--            vocabulary=DBEnumeratedType,
--            title=_("Custom Type"),
--            description=_("Return only items with custom files of this "
++            vocabulary = DBEnumeratedType,
++            title = _("Custom Type"),
++            description = _("Return only items with custom files of this "
                            "type."),
--            required=False),
++            required = False)
+         )
      # Really IPackageUpload, patched in _schema_circular_imports.py
      @operation_returns_collection_of(Interface)
 === modified file 'lib/lp/registry/interfaces/sourcepackage.py'
 --- lib/lp/registry/interfaces/sourcepackage.py	2010-05-17 10:16:03 +0000
 +++ lib/lp/registry/interfaces/sourcepackage.py	2010-05-20 16:13:43 +0000
@@ -187,8 +187,8 @@ class ISourcePackage(IBugTarget, IHasBra
      # in _schema_circular_imports.
      @operation_parameters(
          pocket=Choice(
--            title=_("Pocket"), required=True,
--            vocabulary=DBEnumeratedType))
++            title = _("Pocket"), required = True,
++            vocabulary = DBEnumeratedType))
      # Actually returns an IBranch, but we say Interface here to avoid circular
      # imports. Correct interface specified in _schema_circular_imports.
      @operation_returns_entry(Interface)
@@ -205,9 +205,9 @@ class ISourcePackage(IBugTarget, IHasBra
      # imports. Correct interface specific in _schema_circular_imports.
      @operation_parameters(
          pocket=Choice(
--            title=_("Pocket"), required=True,
--            vocabulary=DBEnumeratedType),
--        branch=Reference(Interface, title=_("Branch"), required=False))
++            title = _("Pocket"), required = True,
++            vocabulary = DBEnumeratedType),
++        branch = Reference(Interface, title = _("Branch"), required = False))
      @call_with(registrant=REQUEST_USER)
      @export_write_operation()
      def setBranch(pocket, branch, registrant):
 === 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 16:02:50 +0000
@@ -14,3 +14,11 @@ from lp.translations.interfaces.potempla
      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 16:19:09 +0000
@@ -1297,8 +1297,8 @@ class POTemplateSet:
          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 @@ class POTemplateSet:
              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 @@ class POTemplateToTranslationFileDataAda
                      if flag
                      ])
--            # Store sequences so we can detect later whether we changed the
--            # message.
--            sequence = row.sequence
--
              # Store the message.
              messages.append(msgset)