Code review comment for lp://staging/~bryce/launchpad/lp-617699-api

> Hi Bryce,
>
> This is good stuff, but needs some more work before it can land.

Thank you for giving this a thorough look through. I've addressed all the issues, but have a question for #8.

> [1]
>
> I think you'll need to get a database review for
> database/schema/comments.sql, database/schema/patch-2208-19-0.sql and
> database/schema/patch-2208-20-0.sql. I think stub and jml are the only
> ones who can grant that.

Actually, those were done on separate branches which landed just today. I probably should have set this merge request as having those as prerequisites.

> [2]
>
> + distro_source_package = exported(
>
> I was suprised to find that this spelling is more common than
> distribution_source_package. No change needed here, just an
> observation.

I was too actually, but I went with this spelling because it helped
prevent overly long lines.

> [3]
>
> + def _getDistroSourcePackage(self):
> + """Retrieves the corresponding source package"""
> + if self.distribution is None or self.source_package_name is None:
> + return None
> + self.distribution.getSourcePackage(
> + self.source_package_name)
>
> This doesn't return the distribution source package, so I'm not sure
> how test_link_distro_source_package succeeds!
>
> Also, as it's intended solely for a property, I think it should be
> named after the property, thus _get_distro_source_package(). I think
> it's clearer if the method naming policy is bent a little here.

I've updated for both these issues, thanks. I don't know why the test
passes either; perhaps when a return type isn't specified, it returns
some value other than None?

> [4]
>
> + def _setDistroSourcePackage(self, distro_source_package):
>
> Same naming suggestion as in [3]. It doesn't matter much though.

Done

> [5]
>
> + distro_source_package = property(_getDistroSourcePackage,
> + _setDistroSourcePackage)
>
> Consider adding a doc parameter, even if it just says "See
> `IBugTrackerComponent`".

Done

> [6]
>
> {{{
> ./lib/lp/bugs/interfaces/bugtracker.py
> 524: E501 line too long (82 characters)
> 524: Line exceeds 78 characters.
> ./lib/lp/bugs/model/bugtracker.py
> 92: 'SourcePackageName' imported but unused
> 89: 'IDistributionSourcePackage' imported but unused
> 770: E501 line too long (80 characters)
> 775: E302 expected 2 blank lines, found 1
> 770: Line exceeds 78 characters.
> ./lib/lp/bugs/tests/test_bugtracker_components.py
> 16: E302 expected 2 blank lines, found 1
> }}}
>
> Please fix this lint.

Fixed

> [7]
>
> make lint also makes the following complaints:
>
> {{{
> database/sampledata/current.sql
> database/sampledata/lintdata.sql differs from
> database/sampledata/current.sql.
> Patches to the schema, or manual edits to database/sampledata/current.sql
> do not match the dump of the launchpad_ftest_template database.
> If database/sampledata/lintdata.sql is correct, copy it to
> database/sampledata/current.sql.
> Otherwise update database/sampledata/current.sql and run:
> make schema
> make newsampledata
> cd database/sampledata
> cp newsampledata.sql database/sampledata/current.sql
> Run make schema again to update the test/dev database.
>
> database/sampledata/current.sql
> database/sampledata/lintdata-dev.sql differs from database/sampledata
> /current-dev.sql.
> Patches to the schema, or manual edits to database/sampledata/current-
> dev.sql
> do not match the dump of the launchpad_dev_template database.
> If database/sampledata/lintdata-dev.sql is correct, copy it to
> database/sampledata/current-dev.sql.
> Otherwise update database/sampledata/current-dev.sql and run:
> make schema
> make newsampledata
> cd database/sampledata
> cp newsampledata-dev.sql database/sampledata/current-dev.sql
> Run make schema again to update the test/dev database.
> }}}
>
> Please look into these.

I reproduced this by running make lint. I re-merged db-devel into this branch after fixing up the other lint issues, then re-ran make lint and it passed with no issue.

> [8]
>
> + distro_source_package = exported(
> + Reference(
> + Interface,
>
> This interface should be IDistributionSourcePackage, shouldn't it? I
> think this is preventing the branch from building.

Yeah I had trouble with this bit. When I include IDistributionSourcePackage
I get this error:

  File "/home/bryce/launchpad/lp-branches/lp-617699-api/lib/lp/scripts/utilities/importfascist.py", line 190, in import_fascist
    module = original_import(name, globals, locals, fromlist, level)
  File "/home/bryce/launchpad/lp-branches/lp-617699-api/lib/lp/bugs/interfaces/bugtracker.py", line 69, in <module>
    from lp.bugs.interfaces.bugtask import (
ImportError: cannot import name BugBranchSearch

But including BugBranchSearch doesn't work either. I'll investigate this more tomorrow when I have fresh brains, but if you have suggestions I'd love to hear them.

> Are there any tests for using this attribute via the web API already
> in the tree? If not, I think it's worth adding something simple.

test_link_distro_source_package() is the only test I've done for this property, but I would like to do some with the web API - where would I place these tests, or where could I look for an example of how it should be done within launchpad?

> [9]
>
> + self.assertTrue(component.distro_source_package is None)
> ...
> + self.assertTrue(component.distro_source_package is not None)
>
> If written as:
>
> self.assertIs(None, component.distro_source_package)
> ...
> self.assertIsNot(None, component.distro_source_package)
>
> these assertions would provide more useful information on failure.

Cool, thanks done.