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

[Kristof Beyls]

Yep it works. It’s actually Jinja’s preferred way: see the note in http://jinja.pocoo.org/docs/dev/templates/#literals.

 

Thanks,

 

Kristof

 

 

From: James Molloy [mailto:james at jamesmolloy.co.uk] 
Sent: 10 June 2015 11:17
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.

 

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/b81e4d0b/attachment.html>