bpo-32890: os: Use errno instead of GetLastError() where appropriate #5784
Conversation
path_error() uses GetLastError() on Windows, but some os functions are implemented via CRT APIs which report errors via errno. This may result in raising OSError with invalid error code (such as zero). Introduce posix_path_error() function and use it where appropriate.
zooba
left a comment
zooba
left a comment
There was a problem hiding this comment.
Just needs the NEWS.d entry so we can merge.
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
posix_path_error() is used only twice, and posix_path_object_error() is used only twice including posix_path_error() implementation. Too much indirections makes harder reading the code and may hide some details. I would inline these function for improving readability and minimizing changes.
|
@serhiy-storchaka My motivation for adding those functions was consistency with existing |
|
|
|
@zooba, @serhiy-storchaka Where are we with this PR? |
|
GH-9984 is a backport of this pull request to the 3.7 branch. |
…uncate() (pythonGH-5784) path_error() uses GetLastError() on Windows, but some os functions are implemented via CRT APIs which report errors via errno. This may result in raising OSError with invalid error code (such as zero). Introduce posix_path_error() function and use it where appropriate. (cherry picked from commit 8346031) Co-authored-by: Alexey Izbyshev <[email protected]>
|
GH-9985 is a backport of this pull request to the 3.6 branch. |
…uncate() (pythonGH-5784) path_error() uses GetLastError() on Windows, but some os functions are implemented via CRT APIs which report errors via errno. This may result in raising OSError with invalid error code (such as zero). Introduce posix_path_error() function and use it where appropriate. (cherry picked from commit 8346031) Co-authored-by: Alexey Izbyshev <[email protected]>
IMHO practicality beats purity here. I concur with @izbyshev ("My motivation for adding those functions was consistency with existing path_error and similar functions.") |
…uncate() (GH-5784) path_error() uses GetLastError() on Windows, but some os functions are implemented via CRT APIs which report errors via errno. This may result in raising OSError with invalid error code (such as zero). Introduce posix_path_error() function and use it where appropriate. (cherry picked from commit 8346031) Co-authored-by: Alexey Izbyshev <[email protected]>
…uncate() (GH-5784) path_error() uses GetLastError() on Windows, but some os functions are implemented via CRT APIs which report errors via errno. This may result in raising OSError with invalid error code (such as zero). Introduce posix_path_error() function and use it where appropriate. (cherry picked from commit 8346031) Co-authored-by: Alexey Izbyshev <[email protected]>
|
Practicality beats purity, but it looks to me that this change opposed to this rule. New functions were introduced just for reasons of purity. It needlessly complicated the code and made it harder to read. "A foolish consistency is the hobgoblin of little minds". |
| except OSError as e: | ||
| self.assertTrue(e.winerror is None or e.winerror != 0) | ||
| else: | ||
| self.fail('No OSError raised') |
There was a problem hiding this comment.
Note for future PRs: self.assertRaises should be used in such cases.
It makes the intent of the test clearer, and prevent adding the else block which lowers test coverage (as it is not reached unless the code is buggy).
There was a problem hiding this comment.
OK, thanks for the advice.
9 participants
path_error() uses GetLastError() on Windows, but some os functions
are implemented via CRT APIs which report errors via errno.
This may result in raising OSError with invalid error code (such
as zero).
Introduce posix_path_error() function and use it where appropriate.
https://bugs.python.org/issue32890