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.)
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?
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.
> * 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.
> >> +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)?
> 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,
simplified the code, changed the doc strings. Have a look and let me
know if you see any problems with what I've changed.
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.
I simplified the duplicated code in run_and_verify_unquiet_status(), in
a previous commit.
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.
I hope you like it.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-08-21 17:48:13 CEST