<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Dec 14, 2015 at 5:39 AM, Tamas Berghammer <span dir="ltr"><<a href="mailto:tberghammer@google.com" target="_blank">tberghammer@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">tberghammer added inline comments.<br>
<span class=""><br>
================<br>
Comment at: packages/Python/lldbsuite/test/lldbtest.py:518<br>
@@ +517,3 @@<br>
</span><span class="">+ if hasattr(func, "categories"):<br>
+ cat.extend(func.categories)<br>
+ func.categories = cat<br>
</span>----------------<br>
<span class="">labath wrote:<br>
> tberghammer wrote:<br>
> > tfiala wrote:<br>
> > > This code could potentially accept a single category and drop the need for the list. It could check the type of the input (i.e. the categories input) and, if a single element rather than a list, it could just cat.append() it.<br>
> > ><br>
> > > That would enable a single argument passed to @add_test_categories()<br>
> > ><br>
> > > Totally minor and you can skip this, it just makes for a nice usability enhancement provided the decorator is documented as such.<br>
> > +1: I think we will (accidentally) call the function by a single string in some case so accepting it would be a good idea.<br>
> The test_categories.validate will catch the case where you forget to put the braces and throw errors at you, so it's not possible to get unexpected behavior here.<br>
><br>
> I may be old-fashioned, but I like when things have a single type, which is obvious from the code. Look at all the cases where we do `callable(bugnumber)` and tell me if it's obvious why a "bug number" should be "callable". Plus, none of our other decorators taking lists have this kind of magic in them (imagine how would this complicate the logic of expectedFailureAll).<br>
><br>
> If we want to provide a simpler syntax for adding a single category, i would go for adding a separate `@add_test_category` decorator for that.<br>
</span>I think the pythonic way is to accept (almost) any type of argument including a single element string and any iterable (list, set, generator expression, etc...). In long term I would like to see all of our decorator taking single and multi argument at the same time but we can make that change later with refactoring all decorator to support it (it can be handled easily by a utility function).<br></blockquote><div><br></div><div>I agree with Tamas on this. We don't get much static-time feedback from dynamic scripting languages, so I'd rather err on the side of making it easier for the user of the feature rather than making them think in a static-language mindset without static-language help to point out misuses. In these cases, where we're talking about "one or more of these" (where these is some kind of argument, like one or more OSes as in oslist) I'd prefer them to take a single or plural literal in the future. We can wrap them in a python sequence helper that handles accepting a singular or plural form of argument.</div><div><br></div><div>e.g.</div><div><br></div><div>def list_of(singular_or_plural_arg):</div><div> # Check type in here, return list/sequence if it is one, otherwise wrap in list.</div><div> ...</div><div><br></div><div>def some_decorator_logic(os_one_or_more):</div><div> # Do something with the singular or plural form of os_one_or_more,</div><div> # Use the list_of() helper to make sure we always treat in sequence context.</div><div> for os in list_of(os_one_or_more):</div><div> # do something with the one or more OS entries.</div><div><br></div><div>Something like that. We probably have a few categories of helpers we'd need (checking if an entry is in a one-or-more set, etc.)</div><div><br></div><div>-Todd</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
<br>
Repository:<br>
rL LLVM<br>
<br>
<a href="http://reviews.llvm.org/D15451" rel="noreferrer" target="_blank">http://reviews.llvm.org/D15451</a><br>
<br>
<br>
<br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature"><div dir="ltr">-Todd</div></div>
</div></div>