Thanks Curro, this looks great. Reviewed-by: Dylan Baker <[email protected]>
On Monday, October 06, 2014 12:02:55 PM Francisco Jerez wrote: > Dylan Baker <[email protected]> writes: > > > I have a couple of style nits and one request for you if you wouldn't > > mind. I don't need to see a v2 since I think these are very simple. > > > > Thanks for your comments Dylan :) > > > 1) Could you put two newlines between toplevel functions, that is our > > style. (PEP8 style) > > OK, fixed that. > > > 2) Could you put your constant data before your function definitions, > > again, that's PEP8 > > I don't think that's going to work, my constant "data" references my > function definitions. Okay, that's fine then > > > 3) could you not put a space between the brace and first character in > > lists and dicts, same as above. > > 4) Could you use the print_function from the __future__ module instead > > of the print statement, I've spent a bunch of time converting the rest > > of the python code to use the print_function already. > > > These two should be fixed now as well, see attachment. > > > I have a few comments, below, but all of those are just suggestions > > > > Otherwise this looks pretty straight forward and decent. > > With those changes, for the python code you have my r-b. > > > > Reviewed-by: Dylan Baker <[email protected]> > > > > On Monday, October 06, 2014 12:00:36 AM Francisco Jerez wrote: > >>[...] > >> +def product(ps, *qss): > >> + """ > >> + Generate the cartesian product of a number of lists of dictionaries. > >> + > >> + Each generated element will be the union of some combination of > >> + elements from the iterable arguments. The resulting value of each > >> + 'name' item will be the concatenation of names of the respective > >> + element combination separated with dashes. > >> + """ > >> + for q in (product(*qss) if qss else [{}]): > >> + for p in ps: > >> + r = dict(p, **q) > >> + r['name'] = '-'.join(s['name'] for s in (p, q) if > >> s.get('name')) > >> + yield r > > > > Is itertools.product() insufficient for this task? > > > It's not sufficient by itself, it returns ordered tuples of elements and > we need each combination to be merged into a single dictionary with the > somewhat ad-hoc handling of the 'name' attribute. Sure, we could call > itertools.product() and transform the returned items as they're > generated, but I find the result slightly less readable than the naive > implementation. Cool. I didn't look too closely, I just wasn't sure if you were aware of that function. > > >>[...] > >> +# > >> +# Test the preprocessor defines. > >> +# > >> +gen('preprocessor', """ > > > > you might consider using """\ instead of just """ so you don't get a > > newline at the start of each test file. > > > > Fixed that, thanks. > > >>[...] >
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
