"Jeremy Whitlock" <jcscoobyrs@gmail.com> writes:
> Hi All,
> I have taken the feedback from Karl and created a new patch and
> accompanying log message. Both can be found attached to Issue 2967:
>
> http://subversion.tigris.org/issues/show_bug.cgi?id=2967
Comments on new patch (which has the same filename as the previous
patch, that was a bit confusing :-) ):
diff_tests.py still has some overly long lines. You can break them
like this:
# 1) Test --xml without --summarize
svntest.actions.run_and_verify_svn(\
None, None,
".*--xml' option only valid with '--summarize' option",
'diff', wc_dir, '--xml')
# 2) Test --xml on invalid revision
svntest.actions.run_and_verify_diff_summarize_xml(\
".*No such revision 5555555",
wc_dir, None, None, None,
None, '-r0:5555555')
and
svntest.actions.run_and_verify_diff_summarize_xml([],
os.path.join(wc_dir,
'iota'),
paths, items,
props, kinds,
'-c2')
In svntest/actions.py, you've defined a new function, which is good,
but I think its OBJECT argument is unnecessary:
def run_and_verify_diff_summarize_xml(error_re_string = [],
object = '',
expected_paths = [],
expected_items = [],
expected_props = [],
expected_kinds = [],
*args):
"""Run 'diff --summarize --xml' with the arguments *ARGS. OBJECT
is the 'diff --summarize --xml' is being ran against.
If ERROR_RE_STRING, the ocmmand must exit with error, and the error
message must match regular expression ERROR_RE_STRING.
Else if ERROR_RE_STRING is None, the subcommand output will be parsed
into an XML document and will then be verified by comparing the parsed
output to the contents in the EXPECTED_PATHS, EXPECTED_ITEMS,
EXPECTED_PROPS and EXPECTED_KINDS. Returns on success, raises
on failure."""
output, errput = run_and_verify_svn(None, None, error_re_string, 'diff',
object, '--summarize', '--xml', *args)
[...]
There's a missing word in the doc string anyway :-), but I'd say just
get rid of the OBJECT argument entirely. The target can simply be in
'args', and the options will just come before the args.
The new file subversion/svn/schema/diff.rnc had a lot of whitespace at
the end. Was that on purpose?
The log message doesn't need to be so verbose. For example:
* subversion/tests/cmdline/diff_tests.py
(diff_summarize_xml): Created unit test to test "--xml"
flag functionality.
(test_list): Added diff_summarize_xml to the list of tests to
run for diff.
Obviously, the new test is for the functionality you're implementing
in this commit, so just say "New test." And obviously, you're adding
that test to 'test_list', because test_list is the label for that
second part, and obviously diff_tests.py is being "run for diff" so...
* subversion/tests/cmdline/diff_tests.py
(diff_summarize_xml): New test.
(test_list): Run it.
Similarly with other parts of the log message.
I'm not saying that the exact wording matters, just that it's good to
avoid redundancies in the log message. If there's a lot of text,
someone might miss something important that really can't be derived
from context.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Nov 6 09:38:27 2007