Yasuhito FUTATSUKI wrote on Wed, 13 May 2020 07:11 +0900: > On 2020/05/10 1:24, Daniel Shahaf wrote: > > Yasuhito FUTATSUKI wrote on Fri, 08 May 2020 20:55 +0900: > >> On 2020/05/08 2:46, Daniel Shahaf wrote: > >>> Yasuhito FUTATSUKI wrote on Thu, 07 May 2020 20:46 +0900: > >>>> I think it is need to escape characters in char *value when we print > >> ^some (not all) > >>>> them as Python's str value. The patch below may work for this purpose, > >>>> but I want someone to write more nice code :) > >>> > >>> How about simply adding the human-readable value in a comment? — > >> > >> It's very nice. One of the reason I don't like my code is just > >> readability of the value of "value". > > > > Sure, in general it's nice for protocols and serialization formats to > > be texty, in order for them to be human-readable and -writable. On > > this instance, however, generating Python string literals that are both > > correct and human-readable seems to me like it'd be an effort spent for > > little gain. (I think there's a good chance that no one will _ever_ > > run entries-dump by hand again once it properly supports Python 3.) > > > > One easy way to make the output nicer is to name the lambda function. > > Yes, it's also one of the reasons that it uses lambda function. > I use it only to reduce the occurence of 'value' in > > e.name = value if isinstance(value, str) else value.decode() > > without using temporary named object, in the first patch. > > Then I updated the patch. Not to use lambda function, I added > a method to set str attribute from bytes object to "Entry" class, > and move its definition to the output of entries-dump to show > what it does.
Thanks. Feel free to commit whichever variant you prefer. They're all functionally identical to one another, so whoever writes the code gets to choose. Review: > +++ subversion/tests/cmdline/entries-dump.c (working copy) > @@ -41,12 +43,41 @@ > +print_prefix() Should this read "(void)" for C89 compatibility? In trunk, empty parentheses in a function definition occur only in platform-specific code and in the definition of svn_swig_pl_get_current_pool(), if I'm grepping correctly. On the other hand, we have 127 instances of "(void)" in a function definition. > { > + puts("class Entry(object):\n" > + " \"Object represents pre-1.6 svn_wc_* entries " > + "used by entries-dump helper\"\n" A couple of tweaks/additions to suggest: """An Entry object represents one node's entry in a pre-1.6 .svn/entries file. Similar to #svn_wc_entry_t, but not all fields are populated. Entry objects are generated by the 'entries-dump' test helper tool.""" > + " if b'' == '':\n" > + " def set_strval(self, name, val):\n" > + " self.__setattr__(name, val)\n" > + " else:\n" > + " def set_strval(self, name, val):\n" > + " self.__setattr__(name, val.decode('utf-8', > 'surrogateescape'))"); Suggest to append a \n to the last line, even though puts() adds its own trailing newline. Adding our own would make a specific type of bug impossible: it won't be possible for anyone to extend the string literal and forget to add the trailing newline on the last incumbent line. Cheers, Daniel