[PATCH][LNT] Add ability to filter machines on daily report page + refactor LNT regression tests to enable properly testing it.

James Molloy james at jamesmolloy.co.uk
Wed Jun 10 03:17:22 PDT 2015


Hi Kristof,

Generally LGTM, but this line looks a bit dodgy:

+{% if selector_part_to_search is none %}

shouldn't None have a capital? Have you checked this works?

James

On Tue, 9 Jun 2015 at 18:03 Kristof Beyls <kristof.beyls at arm.com> wrote:

> Ah, I get your point now – indeed that’s much better.
>
> I’ve attached new versions of the patches will all your comments addressed.
>
>
>
> Thanks!
>
>
>
> Kristof
>
>
>
> *From:* James Molloy [mailto:james at jamesmolloy.co.uk]
> *Sent:* 08 June 2015 15:47
>
>
> *To:* Kristof Beyls; Chris Matthews
> *Cc:* llvm-commits
> *Subject:* Re: [PATCH][LNT] Add ability to filter machines on daily
> report page + refactor LNT regression tests to enable properly testing it.
>
> > 0003 and 0004: Instead of trying to work out the column to search on, I
> think it would be easier if you would just give all cells that are
> searchable some extra class, such as "td-searchable". Then, you wouldn't
> need to parameterize the build_regex macro; it could just always do
> something like "$(this).find('td.td-searchable').text()". How does that
> sound? Especially in 0004, this might simplify things.
>
> If done that way, a mechanism will need to be found to figure out from the
> selected "td" that matches, which is the DOM element that needs
> to be hidden (so far those are all <tr>s containing the <td>). One way or
> another, there needs to be a link between the DOM element that needs
> to be hidden/shown, and which part of it that needs to be searched for the
> regex. As long as this functionality will only hide/show table
> rows and search table cells, the alternative you propose may end up being
> a bit simpler. The way it's implemented now, it's probably a bit
> more generic. I guess that the best solution depends on whether in the
> future we might want to do regex searches to hide/show stuff
> other than table rows and cells.
>
>
>
> Yes I agree, both mechanisms need parameterising somehow. But my suggested
> method would make it possible to implement all our current use-cases with
> just default parameters. I really don't like the way that in your current
> solution we have to store the table layout in two locations - (1) in the
> HTML <table> markup and (2) in the parameters to the macro. Having the JS
> grab the information it needs from the DOM (as long as it's not too costly)
> seems to me to be a better design.
>
>
>
> James
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150610/01ae1c34/attachment.html>


More information about the llvm-commits mailing list