Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get rid of IOError. Use OSError instead #60919

Closed
asvetlov opened this issue Dec 18, 2012 · 15 comments
Closed

Get rid of IOError. Use OSError instead #60919

asvetlov opened this issue Dec 18, 2012 · 15 comments
Assignees
Labels
stdlib Python modules in the Lib dir

Comments

@asvetlov
Copy link
Contributor

BPO 16715
Nosy @terryjreedy, @merwok, @asvetlov, @py-user, @koobs

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = 'https://github.com/asvetlov'
closed_at = <Date 2012-12-25.14:48:26.361>
created_at = <Date 2012-12-18.19:36:56.579>
labels = ['library']
title = 'Get rid of IOError. Use OSError instead'
updated_at = <Date 2013-06-11.05:53:37.431>
user = 'https://github.com/asvetlov'

bugs.python.org fields:

activity = <Date 2013-06-11.05:53:37.431>
actor = 'terry.reedy'
assignee = 'asvetlov'
closed = True
closed_date = <Date 2012-12-25.14:48:26.361>
closer = 'asvetlov'
components = ['Library (Lib)']
creation = <Date 2012-12-18.19:36:56.579>
creator = 'asvetlov'
dependencies = []
files = []
hgrepos = []
issue_num = 16715
keywords = []
message_count = 15.0
messages = ['178130', '178131', '179401', '179541', '179599', '179711', '179747', '179950', '179990', '190702', '190794', '190797', '190799', '190800', '190947']
nosy_count = 6.0
nosy_names = ['terry.reedy', 'eric.araujo', 'asvetlov', 'py.user', 'python-dev', 'koobs']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue16715'
versions = ['Python 3.4']

@asvetlov asvetlov self-assigned this Dec 18, 2012
@asvetlov asvetlov added the stdlib Python modules in the Lib dir label Dec 18, 2012
@pitrou pitrou changed the title Get rid of IOrror. Use OSError instead Get rid of IOError. Use OSError instead Dec 18, 2012
@python-dev
Copy link
Mannequin

python-dev mannequin commented Dec 25, 2012

New changeset 7d69d04522e3 by Andrew Svetlov in branch 'default':
Replace IOError with OSError (bpo-16715)
http://hg.python.org/cpython/rev/7d69d04522e3

@asvetlov
Copy link
Contributor Author

Fixed.

@py-user
Copy link
Mannequin

py-user mannequin commented Jan 8, 2013

found a typo (removed LoadError)

commit 80f2b1273e8c0e1a28fabe537ae9c5acbbcee187
Author: Andrew Svetlov <andrew.svetlov@gmail.com>
Date: Tue Dec 25 16:47:37 2012 +0200

Replace IOError with OSError (bpo-16715)

...

diff --git a/Lib/http/cookiejar.py b/Lib/http/cookiejar.py
index a77dc3f..7928e9b 100644
--- a/Lib/http/cookiejar.py
+++ b/Lib/http/cookiejar.py

...

@@ -1786,7 +1786,7 @@ class FileCookieJar(CookieJar):
             self._cookies = {}
             try:
                 self.load(filename, ignore_discard, ignore_expires)
-            except (LoadError, IOError):
+            except OSError:
                 self._cookies = old_state
                 raise

@asvetlov
Copy link
Contributor Author

It's not a typo.

  1. LoadError is inherited from OSError so LoadError exception is also caught.
  2. Pointed code just resets cookie state and reraises exception, exception type is saved.

The code is correct from my perspective.

@py-user
Copy link
Mannequin

py-user mannequin commented Jan 10, 2013

Andrew Svetlov wrote:

LoadError is inherited from OSError

the comment for the function doesn't tell it
today it's inherited from OSError and tomorrow it may inherit from ANYError

@asvetlov
Copy link
Contributor Author

LoadError was IOError descendant, now OSError is directly specified.
If somebody want to change base class for LoadError he should to update the code in several places in http/cookiejar.py.
The docstring for FileCookieJar.revert directly specifies possible exceptions btw.

@py-user
Copy link
Mannequin

py-user mannequin commented Jan 12, 2013

Zen:
Explicit is better than implicit.
In the face of ambiguity, refuse the temptation to guess.

I assume that someone changes the LoadError class and goes into the revert function. How he can figure out that the cookies should return to the previous state if LoadError was raised ?

Before the change it was explicit and after the change it became implicit.

@asvetlov
Copy link
Contributor Author

Please email python-dev if you think LoadError should be directly specified.

@merwok
Copy link
Member

merwok commented Jan 14, 2013

Source code says:

# derives from OSError for backwards-compatibility with Python 2.4.0
class LoadError(OSError): pass

In 3.0, LoadError could have been made a direct subclass of Exception. Now it’s too late, but a best practice IMO would still be to write LoadError in all new code.

This makes no practical difference, unless test_cookiejar does not check that LoadError is properly raised and caught at the right places. In that case, it may be a little more future-proof to follow py-user’s advice.

@terryjreedy
Copy link
Member

Now that 3.2 is off of maintenance and Idle patches are being applied to both 3.3 and 3.4 (and usually 2.7), I plan to backport the Idle subset of changes to 3.3 so patches are more likely to merge forward without incident. (I have already done one where this change was the only problem.)

@python-dev
Copy link
Mannequin

python-dev mannequin commented Jun 8, 2013

New changeset 2fe64ce5da05 by Terry Jan Reedy in branch '3.3':
bpo-18151, part 1: Backport idlelilb portion of Andrew Svetlov's 3.4 patch
http://hg.python.org/cpython/rev/2fe64ce5da05

@koobs
Copy link

koobs commented Jun 8, 2013

Commit to 3.3 broke at least my FreeBSD buildbot:

http://buildbot.python.org/all/builders/AMD64%20FreeBSD%209.0%20dtrace%203.3/builds/641/steps/test/logs/stdio

Also setting +Version: Python 3.3 on this.

@terryjreedy
Copy link
Member

I don't think so. The idle test, test_idle, passed. The patch did not even affect any of the three idle files that it currently tests. Just because a commit triggers a test does not mean that it is the cause of any failure that happens. The log says the failure was in test_subprocess.
'''
FAIL: test_wait_timeout (test.test_subprocess.ProcessTestCaseNoPoll)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/usr/home/buildbot/python/3.3.koobs-freebsd/build/Lib/test/test_subprocess.py", line 953, in test_wait_timeout
    p.wait(timeout=0.0001)
AssertionError: TimeoutExpired not raised
'''
As I interpret the traceback, this did not fail is the re-run.

I did not set 3.3, nor reopen, because this patch is primarily part of a new issue, bpo-18151. I cross-referenced this one so that if anyone else thought of backporting some of the mega patch, this backport would be recorded here.

@koobs
Copy link

koobs commented Jun 8, 2013

Apologies for the noise Terry, rebuilding passes.

Unsetting versions: 3.3

Is the failure on the build I reported worth opening an issue for?

@terryjreedy
Copy link
Member

I would wait and see if it happens gain or on other buildbots. As I am sure you know, intermittent failures, not reproducible on demand, are nasty.

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir
Projects
None yet
Development

No branches or pull requests

4 participants