On Tue, Oct 3, 2023 at 2:01 AM Daniel Sahlberg <daniel.l.sahlb...@gmail.com> wrote:
> Den tis 3 okt. 2023 kl 02:44 skrev Nathan Hartman < > hartman.nat...@gmail.com>: > >> On Mon, Oct 2, 2023 at 5:39 AM Daniel Sahlberg >> <daniel.l.sahlb...@gmail.com> wrote: >> >> 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: >> > > This looks good to me! > Thanks for the review! Committed in r1912724. More below: Agreed! Being fancy, how about: > > [[[ > Index: subversion/bindings/swig/python/svn/fs.py > =================================================================== > --- subversion/bindings/swig/python/svn/fs.py (revision 1912696) > +++ subversion/bindings/swig/python/svn/fs.py (working copy) > @@ -170,8 +170,13 @@ > + [self.tempfile1, self.tempfile2] > > # open the pipe, and return the file object for reading from the > child. > - p = _subprocess.Popen(cmd, stdout=_subprocess.PIPE, bufsize=-1, > - close_fds=_sys.platform != "win32") > + try: > + p = _subprocess.Popen(cmd, stdout=_subprocess.PIPE, bufsize=-1, > + close_fds=_sys.platform != "win32") > + except FileNotFoundError as e: > + e.strerror = "External diff command not found in PATH" > + raise > + > return _PopenStdoutWrapper(p) > > else: > ]]] > > In my testing that gave me a FileNotFoundError with a custom error message > instead of the generic "No such file or directory". > I see only one issue: FileNotFoundError is new in Python 3, so Python 2 will fail with a NameError when it sees that. However: On Python 3, FileNotFoundError inherits from OSError, OSError exists in Python 2, and OSError in both Pythons has the strerror attribute. So (unless I'm missing something) we should catch OSError instead of FileNotFoundError here for compatibility. (Excuse my C++ terminology, "throw".."catch" :-) Cheers, Nathan
