LGTM, thanks!
On Mon, Feb 24, 2014 at 12:43 PM, Hrvoje Ribicic <[email protected]> wrote: > > > > On Fri, Feb 21, 2014 at 3:52 PM, Petr Pudlák <[email protected]> wrote: > >> >> >> >> On Tue, Feb 18, 2014 at 3:39 PM, Hrvoje Ribicic <[email protected]> wrote: >> >>> The PrintGenericInfo function in cli.py did not handle tuples as >>> containers of items, making it impossible for these to be deserialized >>> automatically when a YAML parser is used. This patch adds separate >>> handling of tuples, including inlining them for readability when >>> possible. >>> >>> Signed-off-by: Hrvoje Ribicic <[email protected]> >>> --- >>> lib/cli.py | 25 ++++++++++++++++++++++++- >>> 1 file changed, 24 insertions(+), 1 deletion(-) >>> >>> diff --git a/lib/cli.py b/lib/cli.py >>> index 951ef4a..97c16ba 100644 >>> --- a/lib/cli.py >>> +++ b/lib/cli.py >>> @@ -4172,6 +4172,16 @@ def CreateIPolicyFromOpts(ispecs_mem_size=None, >>> return ipolicy_out >>> >>> >>> +def _NotAContainer(data): >>> + """ Checks whether the input is not a container data type. >>> + >>> + @rtype: bool >>> + >>> + """ >>> + return not (isinstance(data, list) or isinstance(data, dict) or >>> + isinstance(data, tuple)) >>> >> >> At other places in the code I've seen >> >> isinstance(data, (list, dict, tuple)) >> >> perhaps this could be used as well (but I'm not so familiar with >> isinstance to be 100% sure). >> >> > > Ack, I completely forgot about this syntax, thanks! > > >> + >>> + >>> def _SerializeGenericInfo(buf, data, level, afterkey=False): >>> """Formatting core of L{PrintGenericInfo}. >>> >>> @@ -4217,7 +4227,20 @@ def _SerializeGenericInfo(buf, data, level, >>> afterkey=False): >>> buf.write(key) >>> buf.write(": ") >>> _SerializeGenericInfo(buf, val, level + 1, afterkey=True) >>> - elif isinstance(data, list): >>> + elif isinstance(data, tuple) and all(map(_NotAContainer, data)): >>> + # tuples with simple content are serialized as inline lists >>> + first = True >>> + buf.write('[') >>> + for item in data: >>> + if first: >>> + first = False >>> + else: >>> + buf.write(", ") >>> + buf.write(str(item)) >>> + buf.write(']') >>> + buf.write("\n") >>> >> >> Perhaps this could be simplified something like this (untested): >> >> buf.write("[" + ", ".join(map(str, data)) + "]\n") >> >> or even better >> >> buf.write("[" + utils.CommaJoin(data) + "]\n") >> >> or (as I've seen at some place in our code) >> >> buf.write("[%s]\n" % utils.CommaJoin(data)) >> >> (at slight performance cost.) >> > > Having written this exact code dozens of times, I do not know why I made > this particular choice. Bad day, I guess :) > Will fix! > > >> >> >>> + elif isinstance(data, list) or isinstance(data, tuple): >>> + # lists and tuples >>> if not data: >>> buf.write("\n") >>> else: >>> -- >>> 1.9.0.rc1.175.g0b1dcb5 >>> >>> >> Otherwise LGTM >> > > Thanks for the review, interdiff is: > > diff --git a/lib/cli.py b/lib/cli.py > index 97c16ba..a07417b 100644 > --- a/lib/cli.py > +++ b/lib/cli.py > @@ -4178,8 +4178,7 @@ def _NotAContainer(data): > @rtype: bool > > """ > - return not (isinstance(data, list) or isinstance(data, dict) or > - isinstance(data, tuple)) > + return not (isinstance(data, (list, dict, tuple))) > > > def _SerializeGenericInfo(buf, data, level, afterkey=False): > @@ -4229,16 +4228,7 @@ def _SerializeGenericInfo(buf, data, level, > afterkey=False): > _SerializeGenericInfo(buf, val, level + 1, afterkey=True) > elif isinstance(data, tuple) and all(map(_NotAContainer, data)): > # tuples with simple content are serialized as inline lists > - first = True > - buf.write('[') > - for item in data: > - if first: > - first = False > - else: > - buf.write(", ") > - buf.write(str(item)) > - buf.write(']') > - buf.write("\n") > + buf.write("[%s]\n" % utils.CommaJoin(data)) > elif isinstance(data, list) or isinstance(data, tuple): > # lists and tuples > if not data: > > >
