Code review comment for lp:~gary/launchpad/zbuildout

On August 21, 2009, Gary Poster wrote:
> On Aug 18, 2009, at 5:57 PM, Francis J. Lacoste wrote:

>
> >> === modified file 'lib/canonical/testing/ftests/test_mockdb.py'
> >
> > Instead of commenting everything here, why didn't you simply comment
> > out the
> > line adding the MockDBTestCase to the suite in suite() ? I think
> > this would
> > basically have the same effect. (With a comment explaining why this is
> > disabled.
>
> The reason why was that there were many tests that were still able to
> run. All of the ones that do not rely on the retry stuff still are
> registered. Is that not worthwhile for some reason?

It is. So that's fine.

>
> >> === modified file 'setup.py'
> >> --- setup.py 2009-07-24 11:14:47 +0000
> >> +++ setup.py 2009-08-18 21:42:18 +0000
> >> @@ -20,26 +20,78 @@
> >> maintainer='Launchpad Developers',
> >> description=('A unique collaboration and Bazaar code hosting
> >> platform '
> >> 'for software projects.'),
> >> - license='LGPL v3',
> >> + license='Affero GPL v3',
> >> + # this list should only contain direct dependencies--things
> >> imported or
> >> + # used in zcml.
> >> install_requires=[
> >> 'bzr',
> >> + 'chameleon.core',
> >> + 'chameleon.zpt',
> >> 'feedvalidator',
> >> 'funkload',
> >> 'launchpadlib',
> >> 'lazr.smtptest',
> >> 'lazr.uri',
> >> + 'mechanize',
> >
> > Do we require mechanize? Isn't this an indirect dependency of
> > zope.testbrowser?
>
> We have it here because of testbrowser, but we import it directly.
>
> Here and in the other usage lists below I show you the uses I found
> quickly today, reviewing your questions. There may be more.
>
> lib/lp/code/browser/tests/test_product.py:11:from mechanize import
> LinkNotFoundError
> ./lib/canonical/launchpad/pagetests/standalone/xx-beta-testers-
> redirection.txt:137: >>> from mechanize._clientcookie import Cookie
> ./lib/lp/translations/stories/standalone/xx-sourcepackage-export.txt:
> 31: >>> from mechanize import LinkNotFoundError
> ./lib/lp/soyuz/stories/ppa/xx-ppa-files.txt:127: >>> from mechanize
> import LinkNotFoundError
>
> >>> 'mocker',
> >>> 'oauth',
> >>> 'python-openid',
> >>
> >> 'pytz',
> >> + # This appears to be a broken indirect dependency from
> >
> > zope.security:
> >> + 'RestrictedPython',
> >> 'setuptools',
> >> 'sourcecodegen',
> >> 'storm',
> >> - 'chameleon.core',
> >> - 'chameleon.zpt',
> >> + 'transaction',
> >> + 'wadllib',
> >> 'z3c.pt',
> >> 'z3c.ptcompat',
> >> - 'wadllib',
> >> + 'zc.zservertracelog',
> >> + 'zope.app.appsetup',
> >> + 'zope.app.component',
> >> + 'zope.app.dav', # ./package-includes/dav-configure.zcml
> >
> > Really!?!
>
> Yeah, see that dav-configure.zcml thing. That's why the comment is
> there. Do you want me to try removing that file and then removing the
> dependency?
>
> Unless you are sure we can get rid of that zcml file, I think I'd
> prefer to make a bug for this, asking to investigate, and just leave
> things as they are.
>

That's fine.

>
> Here's the incremental diff so far. Please let me know what you think
> about my replies to the other stuff, particularly the test_mockdb bits.
>

I think you are good to go!

--
Francis J. Lacoste
<email address hidden>