Code review comment for lp:~baztian/bzr/recursive-diff

/me nods@mgz about 2.5 being closed for new features.

As 'meld' can already be used without the @recursive flag, I understand why
you put it in the --diff-options, but I too feel it weird. I think it would
be better to have a dedicated option that can be used *only* when --using is
also used, not sure about which name to use but since diff is already
recursive, may be --tree ?

Overall this is a nice feature to have and I'm glad you're working on it. I
wish we have a way to test it better than a review alone allows as I
suspect ignoring stuff you can't diff (in _write_recursive_files) is likely
to break without more testing. That's just a gut feeling though :-/

By the way, are you sure 'meld' can't display symlinks ?

_write_recursive_file, as a name, sounds funny and does not seem to match
the design.

It seems to me that you want to change DiffTree (or rather subclass it or
something) instead of DiffPath. I.e. really diff trees rather than hacking
how one file is diff'ed.

138 + def assert_diff(self, cmd_args, old_files, new_files):
139 + """Assert the correct files have been extracted for the
140 + revisions to the temp folders.

assertExtractedFiles would be more idiomatic.

150 + def assert_files(self, base_path, files):

Wow, this does a lot ! Worth a docstring and comments ;)

Should it be named assertProducedFiles or something ?

This seems to be a nice set of tests, but they are a bit hard too read. For
one, they seem to duplicate some setup that could go into a ... setUp()
method ;)

You may also want to add tests for symlinks (requiring features.Symlink) if only to check and document how your change behaves.

161 + if os.path.islink(filepath):

Someone knowing windows better than me can comment about whether this will
work or fail there ?

314 + def test_finish(self):

test_finish_cleans_up_temp_files ?

332 + def test_finish(self):

'Eager Test' (http://xunitpatterns.com/Obscure%20Test.html) is bad, you shouldn't do that :) 'Defect Localization' (http://xunitpatterns.com/Goals%20of%20Test%20Automation.html#Defect%20Localization) is good, do it !

Can you split it up with meaningful names ?

Please re-target to 2.6 and let's keep the discussion going, this really
sounds like a very cool feature ;)