Re: svn commit: r25561 - branches/ctypes-python-bindings/csvn

Hi Sage,

Nice patch! In general it looks good.

Instead of asking users to pass in filenames, we could allow users to
pass in Python files to the 'diff' function, and converted these file
objects to APR file objects. Potentially we could just pass in a
temporary file to the diff function and write any output we find there
to the Python file (or StringIO object) that the user passes in. We
already use this trick in the wrapper for SubversionException.

If you look at the interface for 'Repos.cat', you'll see that it's
pretty clean because users can just pass in any old Python file (even
'sys.stdout' or a 'StringIO' object) and we'll write out the output to
that file. I like how clean and easy to use that interface is, and
it'd be great if our 'diff' interface was just as Pythonic.

I'm not sure if the above idea is worth the effort it takes to
implement. It might be more trouble than it's worth, but since I was
thinking about it I thought I'd mention it.

Some stylistic comments below.

> Patch by: me
No need for this, since obviously you're committing the patch.

> + Generated headers will be encoded using HEADER_ENCODING ("" by
> + deafult).

Typo here. s/default/default/

> +
> + If OUTFILE_NAME is provided, then the diff will be outputted to

Typo here. s/outputted/written/

> + if (not outfile_name) | (not errfile_name):

Shouldn't this be a regular "or" instead of a bitwise or?

> + #return_strings can only be True if output is not going to
> + #stdout and stderr.

In Subversion Python code we like to put a space after the '#' mark, like this:
 # return_strings can only be True if output is not going to
 # stdout and stderr.

We should do this throughout the patch, but I won't mention this again.

> + apr_file_open_stdout(byref(outfile),self.iterpool)

We need a space after the comma here.

Cheers,

David

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jun 28 08:19:51 2007