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

[James Molloy]

Hi Kristof,

Comments inline from me.

On Mon, 8 Jun 2015 at 12:40 Kristof Beyls <kristof.beyls at arm.com> wrote:

> Thanks for the quick review James.
>
> Some comments inline below:
>
> > From: James Molloy [mailto:james at jamesmolloy.co.uk]
> > Sent: 08 June 2015 10:41
> > 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.
> >
> > 0001:
> >
> > +                              is not None]
> > You don't need is not None here. .search() will evaluate to a Match
> object or None, which can be treated as a boolean.
>
> Indeed - my preference for explicitness shows up here. I was in doubt on
> writing the explicit "is not None", and you
> picked on it, so I'll remove it before committing.
>
> Thanks.


> > +              }}" rel="stylesheet" media="screen"/>
> >
> > 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.
>
> 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.
> I looked into python's builtin HTML parsing library. It doesn't create a
> tree representation of the document, it only calls
> event handlers when encountering tags. I think having a tree
> representation available is more handy when writing unit tests.
> Python's default XML parser does create such a tree representation, which
> is why I went for using that one, and fixing
> incorrect syntax and changing to XHTML in a few places.
> If there would be a good builtin HTML parser creating tree representations
> in python, I'd be happy to stick to HTML instead of
> XHTML, but I don't think there is a such a parser in the python default
> libraries.
>
>
Ah I see - that makes sense. I think this is fine, then.


> >
> > Otherwise this looks fine to me, and thanks for the tests!
>
> > 0002:
> > +  {{ utils.regex_filter_box('filter', '.searchable tr', "Machines...")
> }}
> >
> > It looks like you've moved this around the page - from before the
> <h3>Machines</h3> to after it. Was this deliberate?
>
> Yes, it is deliberate, as I think the box is in a more logical place when
> moved. But obviously, I shouldn't call this
> a non-functional change, which I claim in the commit message. I'll move
> the moving of the box to a separate commit.
>
> Thanks.


> > +    (function ($) {
> >
> > 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.
> >
> > 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/20150608/ebde7938/attachment.html>