Code review comment for lp:~wallyworld/launchpad/tales-linkify-broken-links

Hi Ian,

The code path firstly looks things up and then determins the url based on what
it found. Instead of remembering everything, you could just determine the
url, especially given that we only care about the trailing at one place.
In fact, I'm not even sure we want to deal with the trailing at all.

        try:
            # first check for a valid branch url
            try:
                branch, trailing = getUtility(IBranchLookup).getByLPPath(path)
                url = canonical_url(branch)
            except (NoLinkedBranch):
                # a valid ICanHasLinkedBranch target exists but there's no
                # branch or it's not visible, so get the target instead
                target = getUtility(ILinkedBranchTraverser).traverse(path)
                userMessage = (
                    "The requested branch does not exist. "
                    "You have landed at lp:%s instead." % path)

They haven't landed at lp:whatever instead. All you need to say is something
like:

    There is no branch linked for lp:whatever.

I'm beginning to wonder if we want to send them to the target at all...

                self.request.response.addNotification(userMessage)
                url = canonical_url(target, rootsite='mainsite')
        except (CannotHaveLinkedBranch, InvalidNamespace,
                InvalidProductName, NotFoundError) as e:
            self.request.response.addErrorNotification(
                    "Invalid branch lp:%s. %s" % (path, e.message))
            url = self.request.getHeader('referer')

That way you don't need the:
    target = branch = trailing = None

And skip the appending of the trailing.

I'm also a little concerned that you are getting a deprecation warning about
e.message. The docs say you should just use __str__, so...
    "Invalid branch lp:%s. %s" % (path, e))
should work fine.