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
Received on 2020-05-13 02:57:41 CEST