bpo-37478: Specify possible exceptions for os.chdir() #14611
Conversation
Whitespace added after new section was causing the document tests to fail, this should fix it.
|
@terryjreedy was unable to manually add you as a reviewer. See my comment on https://bugs.python.org/issue37478 for more details. |
|
I forget that contributors cannot set anything, so sorry about the request. (A change is being discussed. Contributors being able to set headers on bpo issues they open is both a blessing and a nuisance.) I verified Exceptions on 3.7 also. |
Removed double space between a period and the next sentence on line 1599.
|
The travis failure https://travis-ci.org/python/cpython/jobs/554898968 appears to be an unrelated socket failure. I restarted it. |
|
@terryjreedy Oh okay. Perhaps instead of allowing anyone who has previously contributed to the repository to be able to edit labels, it would instead require a role within the Python organization on github which only allows the editing of labels. I'm not sure what the title of the role would be, but it would be especially helpful for anyone who does a significant amount of PR reviews and is not a core developer with commit privileges. Also, I seem to be having some issues with getting the travis-ci test to pass. Initially it looked as if it was caused by whitespace added, but as far I can tell I fixed that issue and it's still not passing. I'll keep looking through it to see if there's anything I missed. Probably the most cumbersome part of contributing to the docs has 8000 been the strictness of the doc-tests. Would be great if there was a way to automatically resolve common formatting and whitespace issues, or if the tests specifically reported the issues found. As far as I can tell, it's just a pass/fail with no specific details on which part of the doc-test that it failed on. Edit: also a quick question, what is required to join the Python GitHub organization? Note sure if it's only core developers, or if it's others as well. Would I be eligible as a PSF contributing member? |
|
"Removed added whitespace" removed added trailing whitespace, which a cpython checkin rule prohibits for .c, .py, and .rst files. tools/scripts/patchcheck.py detects and fixes violations. I run it with check.bat in the directory containing my python repository. Or I check files with git gui, which marks trailing whitespace the same way as in the diff for this commit. You should use one of the two or the equivalent before local commits. If you build docs locally without warning, the Travis test should be no problem. The double space after a . should not have been a issue. Traivis failed again. https://travis-ci.org/python/cpython/jobs/554902228 |
|
@terryjreedy Thanks for the suggestions, I'll try using the recommended tools before committing changes in the future. Most of the PRs I've done so far have been rather simple and I was able to get away without running prior tests, but it would definitely be better to ensure there's no issues before committing. I usually performing tests when making any code-related changes, but I had not thought of doing it for documentation for some reason. After I correct the issues, should I add another commit to this PR or just close it and create a new one? At this point, the commit history seems to have gotten a bit convoluted. Edit: before attempting to make further corrections or closing the PR, I'll wait a bit to see if it's an issue with travis. |
|
@terryjreedy I created a local repository of cpython, with the only difference being the latest changes made in
Since there were no corrections to be made or any errors, it looks like travis was having issues when the tests were performed. Let me know if I need to submit a new PR or if the checks just need to be restarted. Edit: Also it looks like the exception in travis was |
Doc/library/os.rst
Outdated
| @@ -1598,6 +1598,9 @@ features: | |||
|
|
|||
| This function can support :ref:`specifying a file descriptor <path_fd>`. The | |||
| descriptor must refer to an opened directory, not an open file. | |||
|
|
|||
| This function can raise :exc:`OSError`, such as :exc:`FileNotFoundError` and | |||
| :exc:`NotADirectoryError`. | |||
There was a problem hiding this comment.
Another common error is PermissionError. Also, in Windows if a path component exceeds the filesystem's maximum allowed name length or if it contains reserved characters, chdir raises OSError with errno.EINVAL.
It's also noteworthy that ValueError is raised if there's a null ("\x00") in the path. However, it might be better to discuss this generally in the section about filenames.
There was a problem hiding this comment.
@eryksun Thanks for the feedback and suggestions. Since PermissionError falls under the category of OSError exceptions, I'm not entirely certain as whether or not it should be explicitly mentioned. The purpose of that section was primarily to mention that the function could raise OSError exceptions and just to provide a couple of examples rather than explicitly state every possible type of OSError which could be encountered. The section could get excessively verbose rather quickly, see https://docs.python.org/3/library/errno.html#module-errno.
However, PermissionError might be common enough to warrant mentioning it, I'll have to see what the others think. As for the ValueError behavior, since that would apply universally to any file path argument, it should probably be in a more general section (such as the filename section, as you mentioned) rather than in a specific function description.
There was a problem hiding this comment.
I added PermissionError, leaving ValueError for another issue. Can this function raise OSError itself or only subclasses? I assumed the later in my edit.
This function can raise :exc:OSError subclasses such as
:exc:FileNotFoundError, :exc:PermissionError, and exc:NotADirectoryError.
There was a problem hiding this comment.
@terryjreedy Specifically, the function raises the subclasses when it comes to FileNotFoundError and NotADirectoryError, so your assumption was correct. As far as I am aware, it can't directly raise OSError. I verified this is the case for FileNotFoundError and NotADirectoryError in Python 3.9:
There was a problem hiding this comment.
On the commit message for Fix exception tag, it should be PermissionError instead of NotADirectoryError.
Edit: Not sure if I can retroactively change the commit message, or if that has to be done by a core dev. Let me know if there's a way for me to fix it.
There was a problem hiding this comment.
I added PermissionError, leaving ValueError for another issue. Can this function raise OSError itself or only subclasses? I assumed the later in my edit.
As I said in my previous comment, in Windows it can raise OSError with errno == EINVAL (from WINAPI winerror == ERROR_INVALID_NAME) if a path component exceeds the maximum allowed name length or contains characters reserved by the filesystem such as control characters (1-31) and the 5 wildcard characters (*?"<>). Python lacks something like an InvalidArgumentError subclass for this case. This is a generic filename error like the case with embedded null characters and ValueError, so maybe a subsection can briefly discuss invalid filenames in Windows.
There was a problem hiding this comment.
@aeros167 The within PR commit messages do not matter after the fact as commits are squished into 1 for the merge.
@eryksun I commited too soon without fully understanding. I will do a followup PR adding 'and' to say 'OSError and subclasses'. This should be good enough for this issue.
Where ``NotADirectoryError`` was added to the docs for os.chdir(), there was a missing ``:`` behind ``exc:``
|
Travis failed again with the same exception |
|
Given that a copied and pasted, I don't know how I deleted a colon. Too bad since the test passed before. |
|
Thanks @aeros167 for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8. |
|
GH-14629 is a backport of this pull request to the 3.8 branch. |
(cherry picked from commit 0717b4d) Co-authored-by: Kyle Stanley <aeros167@gmail.com>
|
GH-14630 is a backport of this pull request to the 3.7 branch. |
(cherry picked from commit 0717b4d) Co-authored-by: Kyle Stanley <aeros167@gmail.com>
|
@terryjreedy I think it might a markdown issue with copying and pasting certain characters or tags. I had the same thing happen when I tried to copy and paste the section into the bug tracker's issue, where the first colon in the tags were missing. Thanks for regularly checking back on the PR with timely responses. Usually, it seems that the documentation PRs take a bit longer to receive feedback on. |
|
Followup, adding 'and' is #14631. |
Should this be backported to previous versions of the docs or only applied to 3.9? As far as I'm aware, this exception behavior has occurred since earlier versions of Python 3. These exceptions occur as a result of invalid file paths specified, which was added to
os.chdir()in 3.3, according to the docs.https://bugs.python.org/issue37478