Code review comment for lp://staging/~wgrant/launchpad/sensible-validate_target

Revision history for this message
Curtis Hovey (sinzui) wrote :

This branch looks good to land. I have a comment that relates to my own recent work. Maybe your thoughts will help us to make a method faster.

> === modified file 'lib/lp/bugs/model/bugtask.py'
> --- lib/lp/bugs/model/bugtask.py 2011-07-22 04:50:51 +0000
> +++ lib/lp/bugs/model/bugtask.py 2011-07-25 23:06:42 +0000
...
> @@ -439,6 +437,65 @@
> return validate_target_attribute(self, attr, value)
>
>
> +def validate_target(bug, target):
> + """Validate a bugtask target against a bug's existing tasks.
> +
> + Checks that no conflicting tasks already exist.
> + """
> + if bug.getBugTask(target):
> + raise IllegalTarget(
> + "A fix for this bug has already been requested for %s"
> + % target.displayname)
> +
> + if IDistributionSourcePackage.providedBy(target):
> + # If the distribution has at least one series, check that the
> + # source package has been published in the distribution.
> + if (target.sourcepackagename is not None and
> + len(target.distribution.series) > 0):
> + try:
> + target.distribution.guessPublishedSourcePackageName(
> + target.sourcepackagename.name)

I believe this is the method that contributes to bugs
https://bugs.launchpad.net/launchpad/+bug/618366 and
https://bugs.launchpad.net/launchpad/+bug/721643

I tried to avoid using it. Do you know why guessPublishedSourcePackageName()
requires a string and does the extra work to get the spn. We already have
an spn half the time. I also want to avoid calling this method if the method
that provided the spn can be trusted to have provide one that was published
in the distro.

Ah, but this method is smart enough to verify the branch for the package.
I like that.

The binary name is in part the fault of the existing vocabs. I hope
Steven's work will make this path in the method obsolete, or at least very
rare.

review: Approve (code)

« Back to merge proposal