Den sön 1 okt. 2023 kl 21:50 skrev Nathan Hartman <hartman.nat...@gmail.com >:
> On Sun, Oct 1, 2023 at 9:58 AM Daniel Sahlberg < > daniel.l.sahlb...@gmail.com> wrote: > > > > Hi, > > > > Issue SVN-1778 [1] is about the Python bindings, class FileDiff, where > get_pipe() called an external program diff (under Windows: diff.exe). Since > diff.exe has to be installed (and added to PATH) manually on Windows the > function would fail. The same is true on other os:es but diff is probably > more commonly present by default. > > > > r1824410 added support for running the internal Subversion diff > functions (svn.diff.file_diff_2 and svn.diff.file_output_unified4) as long > as the argument diffoptions is None (the default). Since diffoptions is > passed as arguments to the diff command, it seems reasonable to require the > presence of diff (or diff.exe) if one uses diffoptions. > > > > I'm suggesting to close this issue - it is not completely solved but > when using diffoptions,it is probably expected that this arguments is > passed on to whatever diff/diff.exe executable is present on the system and > thus it is not unreasonable to require the end user to install diff.exe > > > > Kind regards, > > > > Daniel Sahlberg > > > > [1] https://issues.apache.org/jira/browse/SVN-1778 > > > 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! > > 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). > > I noticed that there aren't docstrings on most of the functions in > subversion/bindings/swig/python/svn/fs.py, and I don't know whether that's > deliberate. If it is, then the suggested docstring could be a comment > instead. > There are docstrings in the other py files so I suppose this was just overlooked. Kind regards, Daniel
