8000 bpo-37478: Specify possible exceptions for os.chdir() by aeros · Pull Request #14611 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-37478: Specify possible exceptions for os.chdir() #14611

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Jul 7, 2019
Merged

bpo-37478: Specify possible exceptions for os.chdir() #14611

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3e4e456
Merge pull request #1 from python/master
aeros Jul 4, 2019
1094492
Add possible exceptions to os.chdir()
aeros Jul 5, 2019
4780942
"method" to "function"
aeros Jul 5, 2019
c0beb06
Removed added whitespace
aeros Jul 6, 2019
acbddb7
📜🤖 Added by blurb_it.
blurb-it[bot] Jul 6, 2019
f1e3533
Whitespace fix
aeros Jul 6, 2019
7e4953f
Update os.rst
terryjreedy Jul 6, 2019
8b515b3
Fix exception tag
aeros Jul 6, 2019
4b65eeb
add space back and trigger retest
terryjreedy Jul 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add possible exceptions to os.chdir()
  • Loading branch information
@aeros
aeros authored Jul 5, 2019
commit 1094492fde7e3346079fc714fa79dd6fd625c9e4
3 changes: 3 additions & 0 deletions Doc/library/os.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 method can raise :exc:`OSError`, such as :exc:`FileNotFoundError` and
:exc:`NotADirectoryError`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author
@aeros aeros Jul 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author
@aeros aeros Jul 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:
image

Copy link
Contributor Author
@aeros aeros Jul 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


.. versionadded:: 3.3
Added support for specifying *path* as a file descriptor
Expand Down
0