On Mon, Oct 2, 2023 at 5:39 AM Daniel Sahlberg <daniel.l.sahlb...@gmail.com> wrote: > Den sön 1 okt. 2023 kl 21:50 skrev Nathan Hartman <hartman.nat...@gmail.com>: (snip) >> This (somewhat platform-specific) "gotcha" isn't completely obvious from a >> first glance at the code. >> >> Your explanation above makes the failure mode and its reasons more clear. >> >> So, to close SVN-1778, I would suggest only to add some documentation of the >> above fact to the function. I'll be happy to compose a suggested docstring. > > > Please do!
Suggested docstring (also attached as a patch): """Perform diff and return a file object from which the output can be read. When DIFFOPTIONS is None (the default), use svn's internal diff. With any other DIFFOPTIONS, exec the external diff found on PATH, passing it DIFFOPTIONS. On Windows, exec diff.exe rather than diff. If a diff utility is not installed or found on PATH, throws FileNotFoundError. Caveat: On some systems, including Windows, an external diff may not be available unless installed and added to PATH manually. """ More below: >> I don't know how it fails (with a cryptic traceback message?) but if we want >> to get fancy, perhaps a failure could give some user-friendly hint about >> things to check for (such as whether a diff tool is available and >> diffoptions are correct). > > > It fails with a FileNotFoundError error so it should be trivial to catch. I > don't know the proper conventions on error handling in the bindings (or in > Python in general), I guess we should still throw an error, hoping that > whoever use the FileDiff class will catch it and handle in an intelligent way. > > Another way would be to use the internal diff in case of an exception, but it > would be less apparent (I would rather raise an error, even if it just means > ignoring the FileNotFoundError and documenting the cause). In this case, I would rather leave the implementation as it is currently. I think that calling the internal diff when an external diff is expected (with some unknown options) would be worse than failing with FileNotFoundError here. Even if the exception is not caught, the docstring should provide the useful hint. :-) Cheers, Nathan
SVN-1778-patch1.patch
Description: Binary data
