Julian Foad wrote:
> Neels, I've committed a tweaked version of this patch now, in r32617.
>
> I have two questions remaining:
>
> 1. In dump_tree_script__crawler() I don't understand why we explicitly
> avoid printing the root node. (See below for a more detailed note.)
s.b.
>
> 2. I wonder if all the calls to dump_tree_script() in actions.py would
> be better as a single call inside compare_trees(). What do you think?
checking...
I was gonna say that compare_trees() doesn't know the name of the tree kind
and needs another parameter to be able to print e.g. "DISK TREE"; But, the
parameter is already there, all the more I agree, it should be moved inside.
However, there is also another way present in which trees are (sometimes)
printed (only node names, using dump_tree() via verify.display_trees()).
These are also not included in compare_trees(). (all in actions.py)
Maybe this should be unified?
If so, I suggest replacing dump_tree() with dump_tree_script() altogether,
and printing both the actual and expected trees, via calling
verify.display_trees(), which then calls dump_tree_script(). This combines
both current ways. But we might go off on a tangent with this one...
>
> The rest of the notes below are about things I tweaked.
>
>
> On Thu, 2008-08-21 at 03:36 +0200, Neels Janosch Hofmeyr wrote:
>> A new version of the patch. Replies below.
>>
>> [[[
>> Make the cmdline tests print out the complete actual tree and meta-
>> data, as python script, in case of a node mismatch in output, disk,
>> status or skip tree.
>>
>> * subversion/tests/cmdline/svntest/tree.py
>> (print_script): New function in SVNTreeNode class, prints out the node
>> metadata as python script line.
>> (dump_tree_script): New function, prints a whole tree as py script,
>> using SVNTreeNode.print_script().
>
> You should mention all global-scope symbols that are affected, so
> include:
>
> (dump_tree_script__crawler): New function.
Uh, that hurts. Did I not mention it!?
>
>> * subversion/tests/cmdline/svntest/actions.py
>> (run_and_verify_checkout, run_and_verify_export, verify_update,
>> run_and_verify_merge2, run_and_verify_commit, run_and_verify_status,
>> run_and_verify_unquiet_status): Print the output / disk / status / skip
>> tree as py script, if tree comparison throws an exception, using
>> dump_tree_script() from tree.py.
>> ]]]
>
>
>>>> + def print_script(self, stream = sys.stdout, strip = 0):
>>>> + "Python-script-print the meta data for this node."
>>> Please make the doc-string say what each of the arguments means. (Well,
>>> 'self' is probably an exception. The 'stream' argument is pretty obvious
>>> but only because of its default value which might be changed in future,
>>> so it should be mentioned.)
>> Hm, I think I just copied the pprint() function from just above and adjusted
>> as needed. I never paid attention to these things... I figured it's
>> committed, so I can just copy it. Tidying up...
>
> Oh, yes. It's a pain when you want to copy code that already exists but
> isn't very well documented or isn't very well written. Thanks for your
> update.
>
> I have take a hint from you and updated the doc string pprint() in an
> earlier commit.
Nice.
>
>
>>>> +def dump_tree_script(n, strip=0):
>>>> + "Print out a python script representation of the tree's structure."
>>> The doc-string should say what the argument "n" is and what the argument
>>> "strip" is.
>> Hm, "n" isn't documented in dump_tree, either.
>>>> + # Then, there are the props. I never needed to validate them in
>>>> + # any of my tests. I havent checked how they would be scripted.
>>>> Maybe
>>>> + # like self.atts above?
>>>> + #
>>>> + # for name in self.props:
>>>> + # if comma:
>>>> + # line += ", "
>>>> + # line += "%s='%s'" % (name, self.props[name])
>>>> + # comma = True
>>> Yes, I expect so, but I haven't investigated.
>> It's actually a dictionary parameter "props={ 'foo' : 'bar', ... }".
>>
>>>> + line += "),"
>>>> + print >> stream, line
>>>
>> Fixed mine.
>
> Thanks.
>
>
>>>> + if n.name != root_node_name:
>>>> + if strip < len(n.path):
>>>> + n.print_script(strip = strip)
>>> I'm not sure about this block. It's not printing anything about the root
>>> node? And it's not printing anything about nodes whose path is less than
>>> or equal to the "strip" argument"?
>
> I understand now what the "strip" argument was doing.
>
> I still don't know why you don't want to print the root node. Sometimes
> the root node is where the mis-match occurs. Maybe I'm getting confused
> between the root of the tree "N" (called "__SVN_ROOT_NODE") and the root
> node of the sub-tree that we're printing (which may be called "/A" or
> whatever)?
The root node in the sense of the if-condition always has a fabricated name
of "__SVN_ROOT_NODE" and is (mostly) of no concern to the expected trees. It
symbolizes the parent directory, which is usually not checked for. If it is,
it is denoted by a path of "", not "__SVN_ROOT_NODE", so it makes no sense
to print that.
That one tree kind where a root node (` "" : Item(), ') is required, the
test developer has to figure out herself, as far as my patch is concerned.
In the other three tree kinds, the root node is not of concern and actually
confuses the output. I went with the majority of cases.
>
>> I have once over seriously looked at the code structure. My!
>> It's also matching strings now instead of relying on string lengths. That
>> fixed some potential problems and brought some clarity.
>
> Good. I have tweaked it a bit more: made the output lines indented,
I had thought about that and decided to leave indenting to the developer: I
wanted to avoid pushing long lines over the screen width with mere
whitespace. When I paste into vim with autoindent on, the indenting is
magically corrected. Hm, are you trying to say I can't assume everyone is
using vim with autoindent on? ;)
> In 'actions.py':
>
> Best to only print it if tree comparison throws the particular exception
> "tree.SVNTreeError", like the existing cases already do, rather than any
> exception. Done.
ok -- I had assumed any exception while comparing trees must be related to
the trees.
> I simplified the duplicated code in run_and_verify_unquiet_status(), in
> a previous commit.
And, nice, you spotted that it's not an OUTPUT tree but a STATUS tree. It
keept saying "output" all around, so I got it wrong. Ah, that got corrected,
too :)
Though, "STATUS OUTPUT TREE" is still confusing, given that there also is
something called an "OUTPUT TREE". I usually wildly scan around until I see
one of the words "OUTPUT", "DISK", "STATUS" or "SKIP". "STATUS TREE" would
be better, I'd say, since it doesn't say "OUTPUT" in it. Hm, that also
applies to "DIFF OUTPUT TREE", here:
>
> The label "ACTUAL DIFF OUTPUT" made me think, "This doesn't look like
> the output from a diff command!". I added "TREE" to the end of it.
lol :)
Weird how I keep not seeing these things reading my own patches...
I'll try to pay even closer attention in future.
>
> I hope you like it.
It's the good stuff :)
>
> - Julian
>
>
--
Neels Hofmeyr -- elego Software Solutions GmbH
Gustav-Meyer-Allee 25 / Gebäude 12, 13355 Berlin, Germany
phone: +49 30 23458696 mobile: +49 177 2345869 fax: +49 30 23458695
http://www.elegosoft.com | Geschäftsführer: Olaf Wagner | Sitz: Berlin
Handelsreg: Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
Received on 2008-08-22 00:28:17 CEST