launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Hi

Thanks for the most excellent review. It's the most thorough one I've
had. I've addressed most of the issues except for the one item - see my
comments. The import issues were due to the "curse" of having an IDE
that auto imports missing references and it guessed wrong. A small price
to pay given all the other great stuff it does.

> 
> I think __metaclass__ and __all__ normally appear first in Launchpad
> source, followed by imports. I think utilities/format-imports expects
> that too (and you should also run this on your changed files, or use
> utilities/format-new-and-modified-imports -r submit: as a convenience).
>

Thanks for the tip - I had no idea those scripts existed.


> 
> [6]
> 
> +            path = link.strip('/')[len('+branch/'):]
> 
> Is it possible that the branch link does not begin with +branch? I'm
> not used to seeing branches with +branch in their URLs.
>

No, all branch links processed by this new functionality begin with
+branch. See line 276 of stringformatter.py:

    url = '/+branch/%s' % path


> 
> [11]
> 
> Is it likely that we hit any query length problems when requesting the
> status of a page with many links?
>

I don't think so? A post operation is used so I thought that would have
it covered?


> 
> [13]
> 
> +function harvest_links(Y, links_holder, link_class, link_type) {
> 
> I think this function would be cleaner if it returned the link_info
> array instead of putting it into links_holder. That way it need only
> accept two arguments, Y and link_class. Callers should check if the
> returned array is empty.
>

The code has been written so support processing for than just one type
of link. At the moment, there's only branch links. But there will also
be bugs links etc. So the code will look like:

var links_to_check = {}

harvest_links(Y, links_to_check, 'branch-short-link', 'branch_links');
harvest_links(Y, links_to_check, 'bug-link', 'bugs_links');
etc

So we are building a dict mapping link_type_name->[hrefs]

The review comment would be valid if we only ever were going to process
the one type of link. I think the implementation as written is ok given
the above explanation?

> 
> [14]
> 
> Also, given [13] and the presence of the YUI collection module,
> harvest_links() could be much shorter and probably quicker:
> 
>     return Y.Array.unique(
>         Y.all('.' + link_class).get('href'));
> 
> To get the collection module you'll need to add the following in the
> right place (I added it at line 108) in base-layout-macros.pt:
> 
>      <script type="text/javascript"
>             tal:attributes="src string:${yui}/collection/collection.js"></script>
>

I think Array.unique() is only available in YUI 3.2 and at the time of
writing we were not using that version yet. I originally tried to use
Array.unique() only to be disappointed :-(


> 
> [15]
> 
> +                    alert(msg);
> 
> Really? Just checking :)

I wanted to give the user feedback why the "link" they just clicked on
didn't seem to work instead of just swallowing the onclick().

> [16]
> 
> In TestBranchBugLinks (which should probably be renamed), instead of
> creating a new question via the browser, consider creating a new
> question using the object factory (or a bug, doesn't matter). It'll
> probably be a lot quicker.
> 

The factory method for making a question tries to notify() and this
fails since the test is not set up for that (and it is unnecessary).
So I made a bug instead.

> [19]
> 
> +        # XXX wallyworld 2010-10-20 - why oh why is windmill borked?
> +        # Windmill refuses to do the ajax call so these asserts fail :-(
> +        # It all works fine outside of windmill.
> 
> Please add a bug here according to
> https://dev.launchpad.net/PolicyandProcess/XXXPolicy.
> 

It's a tough one. I don't think it's our bug, hence there's nothing to
file in lp. I could create a windmill bug but proving it's a problem
with their code and not just some environmental issue with our tests of
whatever is going to be very difficult :-(

*news flash* - see below

> Mmm, I actually think this needs to be resolved or worked-around
> because this test doesn't show much without it right now.
>

I tried for way too many hours to work around this with no luck. Long
story, I'd be happy to discuss what I tried. I ran it by my team and
didn't find a solution. I also asked a windmill developer, but the
suggestion he gave made no sense to us. Some are even advocating
abandoning windmill for jstestdriver. So there's a can of worms there.

*news flash* - problem "solved" wtf. By changing the test to creating a
bug instead of a question, it works in windmill. Go figure. But it works
now \o/



-- 
https://code.launchpad.net/~wallyworld/launchpad/improved-broken-link-handling/+merge/37095
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/improved-broken-link-handling into lp:launchpad/devel.