On Wed, Oct 4, 2023 at 10:59 AM Yasuhito FUTATSUKI <futat...@yf.bsdclub.org> wrote:
> Hello, > > On 2023/10/04 18:02, Daniel Sahlberg wrote: > > Den ons 4 okt. 2023 kl 06:58 skrev Nathan Hartman < > hartman.nat...@gmail.com > >> : > > > >> Thanks for the review! Committed in r1912724. More below: > >> > > > > Great! > > > > > >> 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. > >> > > > > Good point. If we catch OSError we should check for err.errno = > > errno.ENOENT (as is done in subversion/bindings/swig/python/tests/fs.py). > > > > I don't think anything has formally been decided regarding Python 2 > > support, we have tried hard to keep Python 2 compatibility in 1.14 but > for > > /trunk (and a coming 1.15 release) my opinion is that we should remove > it. > > This should probably be broken out to a separate thread and documented > > somewhere. > > > > I think catching FileNotFoundError is cleaner than OSError + check for > > ENOENT. Also there is no immediate need to backport to 1.14 (this is > just a > > better error message). With that in mind, I'm leaning towards keeping > > FileNotFoundError (we should probably update tests/fs.py to follow the > same > > pattern). > > > > Yasuhito / Jun: Since you are Python experts, do you have any comments? > > Nothing but I don't want that we actively drop Python 2 support, > at least SWIG Python bindings even in trunk, at least now. I agree we shouldn't break Python 2 support if it's currently working. At least, let's delay breaking it until 1.14.x is EOL, to avoid breakage on very long term support OS. (I think that if we start removing Py2 compat on trunk now, we risk mistakenly backporting the changes to 1.14.x at some later time.) However, I see the point that it would be nice to make the code clear and not require us to remember that OSError needs to change to FileNotFoundError in the future. We could add a Python2 check and handle it differently. In the future when we decide to actively remove Python2 support, we can grep for all the Python2 checks and remove that code. I know it makes the code long-winded. :-/ Yasuhito and Jun know much more than me about Python so maybe there is a better way. Cheers Nathan
