<div dir="ltr">Hi Kristof,<div><br></div><div>Comments inline from me.<br><br><div class="gmail_quote"><div dir="ltr">On Mon, 8 Jun 2015 at 12:40 Kristof Beyls <<a href="mailto:kristof.beyls@arm.com">kristof.beyls@arm.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks for the quick review James.<br>
<br>
Some comments inline below:<br>
<br>
> From: James Molloy [mailto:<a href="mailto:james@jamesmolloy.co.uk" target="_blank">james@jamesmolloy.co.uk</a>]<br>
> Sent: 08 June 2015 10:41<br>
> To: Kristof Beyls; Chris Matthews<br>
> Cc: llvm-commits<br>
> Subject: Re: [PATCH][LNT] Add ability to filter machines on daily report page + refactor LNT regression tests to enable properly testing it.<br>
><br>
> 0001:<br>
><br>
> + is not None]<br>
> You don't need is not None here. .search() will evaluate to a Match object or None, which can be treated as a boolean.<br>
<br>
Indeed - my preference for explicitness shows up here. I was in doubt on writing the explicit "is not None", and you<br>
picked on it, so I'll remove it before committing.<br>
<br></blockquote><div>Thanks.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + }}" rel="stylesheet" media="screen"/><br>
><br>
> Is there any particular reason you've XHTML'd up these tags? It looks like there's a lot of HTML cleanup in this patch that probably shouldn't be there. Stuff like adding </tr> I don't think anyone can argue with (just commit that?) but adding /> to the end of tags isn't HTML, it's XHTML and is not normally done, especially for img,link and meta tags.<br>
<br>
The reason I did this is to be able to easily parse the page using python's built-in XML parser in the unit tests I've added.<br>
I looked into python's builtin HTML parsing library. It doesn't create a tree representation of the document, it only calls<br>
event handlers when encountering tags. I think having a tree representation available is more handy when writing unit tests.<br>
Python's default XML parser does create such a tree representation, which is why I went for using that one, and fixing<br>
incorrect syntax and changing to XHTML in a few places.<br>
If there would be a good builtin HTML parser creating tree representations in python, I'd be happy to stick to HTML instead of<br>
XHTML, but I don't think there is a such a parser in the python default libraries.<br>
<br></blockquote><div><br></div><div>Ah I see - that makes sense. I think this is fine, then.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
><br>
> Otherwise this looks fine to me, and thanks for the tests!<br>
<br>
> 0002:<br>
> + {{ utils.regex_filter_box('filter', '.searchable tr', "Machines...") }}<br>
><br>
> It looks like you've moved this around the page - from before the <h3>Machines</h3> to after it. Was this deliberate?<br>
<br>
Yes, it is deliberate, as I think the box is in a more logical place when moved. But obviously, I shouldn't call this<br>
a non-functional change, which I claim in the commit message. I'll move the moving of the box to a separate commit.<br>
<br></blockquote><div>Thanks.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + (function ($) {<br>
><br>
> I'm not sure why this nested function is necessary, but it was just copy-paste so I won't mention any more about it.<br>
><br>
> 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.<br>
<br>
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<br>
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<br>
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<br>
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<br>
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<br>
other than table rows and cells.<br>
<br></blockquote><div><br></div><div>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.</div><div><br></div><div>James</div></div></div></div>