gh-47169: Adding errno Symbolic Values to OSError.__str__()#150041
gh-47169: Adding errno Symbolic Values to OSError.__str__()#150041sadrasabouri wants to merge 19 commits into
OSError.__str__()#150041Conversation
Note: in the tests. I could only verify failure of `test_signal`; `test_exceptions` required windows and `test_tabnanny` appears not to check the error message properly and resulting into false positive.
|
All commit authors signed the Contributor License Agreement. |
|
The "Check if generated files are up to date" failure is real / needs to be addressed: (Should be able to run that locally as |
| @@ -0,0 +1,7 @@ | |||
| OSError.__str__() now includes the symbolic name of the error code (e.g. | |||
There was a problem hiding this comment.
- Be more concise in this. In particular the high level change is just "Include error name in OSError". The particular implementation details aren't critical to describe / important to someone just trying to figure out what changed in CPython.
- Should use rST syntax, in particular when referring to specific classes, methods, or values link to them when appropriate
| open(filename) | ||
| self.assertEqual( | ||
| str(cm.exception), | ||
| f"[Errno {errno.ENOENT} ({errname})] {errmsg}: {filename!r}", |
There was a problem hiding this comment.
nit: I'd prefer being a constant string for the test here, that it gets the integer + ENOENT string is more important to me than "it matches this other piece we can look up"
| not in err): | ||
| raise AssertionError(err) | ||
| if ('OSError: [Errno %d]' % errno.EBADF) not in err: | ||
| if ('OSError: [Errno %d (%s)]' % |
There was a problem hiding this comment.
This exists in the CPython codebase; that makes me worry this same sort of string matching may exist in many other codebases and will be broken by this change...
Is there some way we could measure/check for that? Keeping the symbolic name outside the [] would limit breakage some.
There was a problem hiding this comment.
Good point. It's a valid concern indeed. I ran rg -n '\[Errno [^]]*\]|OSError: \[Errno' Lib Tools Objects Modules Python and found cases treating the int after Error as a format.
The two other alternatives are as follows, let me know which one looks better:
[Errno 2] (ENOENT) No such file or directory[Errno 2] No such file or directory (ENOENT)
I like 1 bettter, to avoid confusion.
|
Hey @cmaloney, I applied the requested changes and raised a question. Once you send me your feedback I will continue on this PR. |
|
I do not like this as this can break tests and other doctests. I think the discussion should be put at DPO again (to me the issue has no consensus at all) |
4 participants
OSError.__str__now includes the symbolic errno name alongside the number (something like below):The errno constants list (previously inlined in
Modules/errnomodule.c) is extracted intoObjects/errnonames.has a macro header. Botherrnomodule.c(for module registration) andObjects/exceptions.c(for name lookup) include it with their customly definedadd_errcodedefinition, avoiding duplication of the errno. list. If the errno value is not in the list (e.g. a platform-specific code not covered by the header), the output falls back to the plain number, same as before.. A new helperOSError_format_errnowalks the list to find the symbolic name for a given errno value.OSError_strcalls this helper in all paths.Tests are adapted from #14988.
Note:
_errno_listis searched linearly on everyOSError.__str__()call. A hash table or a flat array indexed by errno value would give O(1) lookup, but each has its own trade-off. I leave the review open on that, so we can discuss to find the best solution.Question: The tests in test_tabnanny wrap the helper with
self.assertRaises(SystemExit), tabnanny.errprint() writes to stderr and immediately callssys.exit, which raises SystemExit beforeverify_tabnanny_check()reaches theassertEqualonstderr.getvalue.