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

Specify possible exceptions for os.chdir() #81659

Closed
aeros opened this issue Jul 1, 2019 · 14 comments
Closed

Specify possible exceptions for os.chdir() #81659

aeros opened this issue Jul 1, 2019 · 14 comments
Assignees
Labels
3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes docs Documentation in the Doc dir type-feature A feature request or enhancement

Comments

@aeros
Copy link
Contributor

aeros commented Jul 1, 2019

BPO 37478
Nosy @terryjreedy, @miss-islington, @aeros
PRs
  • bpo-37478: Specify possible exceptions for os.chdir() #14611
  • [3.8] bpo-37478: Specify possible exceptions for os.chdir() (GH-14611) #14629
  • [3.7] bpo-37478: Specify possible exceptions for os.chdir() (GH-14611) #14630
  • bpo-37478: Add missing 'and'. #14631
  • [3.8] bpo-37478: Add missing 'and'. (GH-14631) #14632
  • [3.7] bpo-37478: Add missing 'and'. (GH-14631) #14633
  • 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/terryjreedy'
    closed_at = <Date 2019-07-07.04:39:04.578>
    created_at = <Date 2019-07-01.22:40:23.117>
    labels = ['3.8', 'type-feature', '3.7', '3.9', 'docs']
    title = 'Specify possible exceptions for os.chdir()'
    updated_at = <Date 2019-07-07.05:11:18.136>
    user = 'https://github.com/aeros'

    bugs.python.org fields:

    activity = <Date 2019-07-07.05:11:18.136>
    actor = 'terry.reedy'
    assignee = 'terry.reedy'
    closed = True
    closed_date = <Date 2019-07-07.04:39:04.578>
    closer = 'aeros'
    components = ['Documentation']
    creation = <Date 2019-07-01.22:40:23.117>
    creator = 'aeros'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 37478
    keywords = ['patch']
    message_count = 14.0
    messages = ['347080', '347086', '347371', '347395', '347399', '347450', '347458', '347459', '347460', '347462', '347463', '347464', '347466', '347467']
    nosy_count = 4.0
    nosy_names = ['terry.reedy', 'docs@python', 'miss-islington', 'aeros']
    pr_nums = ['14611', '14629', '14630', '14631', '14632', '14633']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue37478'
    versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']

    @aeros
    Copy link
    Contributor Author

    aeros commented Jul 1, 2019

    In the section describing the functionality of os.chdir(), there is no mention of possible errors that can be raised. This differentiates significantly from the sections of other methods, such as os.is_dir():

    "This method can raise :exc:`OSError`, such as :exc:`PermissionError`, but :exc:`FileNotFoundError` is caught and not raised."

    For the purpose of consistency and providing additional useful information, I would suggest doing the same for os.chdir(). os.chdir() differs from os.is_dir() by directly raising the OSErrors FileNotFoundError and NotADirectoryError rather than handling them. I would suggest the addition of the following in os.chdir()'s section:

    "This method can raise :exc:`OSError`, such as exc:`FileNotFoundError` if the *path* does not lead to a valid file or exc:`NotADirectoryError` if the file specified is not a valid directory."

    or a less verbose alternative:

    "This method can raise :exc:`OSError`, such as exc:`FileNotFoundError` and exc:`NotADirectoryError`."

    If a user is reading the documentation, they might naturally assume that os.chdir() catches the FileNotFoundError, similar to the behavior in os.is_dir(). Also, the NotADirectoryError is not explicitly mentioned anywhere else on the "os.rst" page. If it were to be mentioned anywhere, os.chdir() would probably be the most relevant location to do so.

    Also, as a general question, what exactly is the standard for mentioning errors in the method sections for the Python documentation? For the purposes of clarity, I think it would be quite useful to explicitly mention errors, and a brief description of situations which raise them. If the behavior of the errors is consistent across of a group of methods, it can instead be mentioned in the header section.

    However, if the behavior not consistent, such as in this case, I think it is useful to briefly mention how the method deviates. It also seems useful to mention the error at least once if it is not explicitly mentioned elsewhere on the page, even if the parent is mentioned, such as with NotADirectoryError and OSError. This seems to be particularly important to do for the docs in a language such as Python, as it does not require the raise-able/throw-able errors to be mentioned in the code.

    I have already created a branch in my local fork of the cpython repository which implements this change, but I'll await feedback before submitting the PR.

    @aeros aeros added 3.9 only security fixes docs Documentation in the Doc dir type-feature A feature request or enhancement labels Jul 1, 2019
    @aeros
    Copy link
    Contributor Author

    aeros commented Jul 1, 2019

    Minor clarifications:

    This change is referring to the "Files and Directories" section of the "os.rst" page in "cpython/Doc/library/os.rst".

    Also, I realized that I forgot to add the colon before "exc" in a couple locations. This was just a typo when writing the comment, it is correctly formatted in my local branch.

    @terryjreedy
    Copy link
    Member

    what exactly is the standard for mentioning errors
    My impression is that there is none, or that it is inconsistent, but that the trend is to say more. If you want to followup, check the documentation chapters of the devguide, and if there is nothing clear now, post to the pydev list.

    https://docs.python.org/3/library/os.html#os-file-dir has

     os.chdir(path)
        Change the current working directory to path.
        This function can support specifying a file descriptor. The descriptor must refer to an opened directory, not an open file.

    Most of the other entries in do not mention errors, but is_dir has the line mentioned. I think the condensed line is enough to add, as the sub-exception names are self-explanatory; and when they occur, the full message certainly is. (I verified both possibilities mentioned.) Most users should just catch OSError, if anything.

    Make a PR with the short message and request me as a reviewer.

    @aeros
    Copy link
    Contributor Author

    aeros commented Jul 5, 2019

    Thanks for the feedback terry. Do you think it would be helpful to work through some of the other commonly used functions in OS, adding condensed explanations of exceptions which can be raised? In general, it seems quite useful for users to be able to figure out the exceptions in which they may need to account for when using each of the functions.

    Also, should the naming convention be "method" or "function"? The two roughly refer to the same thing, but as far as I'm aware, the convention in Python is to refer to them as "functions". I'm used to calling them "methods" from my prior experience with OOP, so that's why I did so in my initial condensed description. On the docs for OS, there seems to be a few instances where the term "method" is used.

    Personally, I don't mind the usage of either term, at the end of the day it's just semantics. But for the purposes of documentation, consistency seems preferable. Should I create a PR for replacing the mentions of "method" with "function"? Since it's quite minor and in the same OS.rst file, should I link it to this issue instead of creating a new one?

    @aeros
    Copy link
    Contributor Author

    aeros commented Jul 5, 2019

    Actually, I don't believe that I have the appropriate permissions to manually specify reviewers for PRs in the cpython repository. Do I have to be added a member of the Python organization on GitHub for this? I'm registered as a PSF contributing member, but I don't think that alone translates to any repository permissions. Could I be added as a member with low level permissions such as editing issue labels without being granted commit? This would also be helpful for adding "skip news" labels on any other issues, I've been doing a decent amount of PR reviews recently.

    @terryjreedy
    Copy link
    Member

    Please ask your questions about doc conventions, both about exceptions and function/method. Either someone will point to something in the devguide, or we might get a discussion about something that should be added. I would not merge more extensive changes without some feedback.

    @terryjreedy terryjreedy added 3.7 (EOL) end of life 3.8 only security fixes labels Jul 6, 2019
    @terryjreedy terryjreedy assigned terryjreedy and unassigned docspython Jul 6, 2019
    @terryjreedy
    Copy link
    Member

    New changeset 0717b4d by Terry Jan Reedy (Kyle Stanley) in branch 'master':
    bpo-37478: Specify possible exceptions for os.chdir() (GH-14611)
    0717b4d

    @terryjreedy
    Copy link
    Member

    New changeset 4e6bfc4 by Terry Jan Reedy (Miss Islington (bot)) in branch '3.8':
    bpo-37478: Specify possible exceptions for os.chdir() (GH-14611) (GH-14629)
    4e6bfc4

    @terryjreedy
    Copy link
    Member

    New changeset 1dd6507 by Terry Jan Reedy (Miss Islington (bot)) in branch '3.7':
    bpo-37478: Specify possible exceptions for os.chdir() (GH-14611) (GH-14630)
    1dd6507

    @terryjreedy
    Copy link
    Member

    New changeset a9b40e4 by Terry Jan Reedy in branch 'master':
    bpo-37478: Add missing 'and'. (GH-14631)
    a9b40e4

    @miss-islington
    Copy link
    Contributor

    New changeset e841a54 by Miss Islington (bot) in branch '3.7':
    bpo-37478: Add missing 'and'. (GH-14631)
    e841a54

    @miss-islington
    Copy link
    Contributor

    New changeset e414aa9 by Miss Islington (bot) in branch '3.8':
    bpo-37478: Add missing 'and'. (GH-14631)
    e414aa9

    @aeros
    Copy link
    Contributor Author

    aeros commented Jul 7, 2019

    Exceptions for os.chdir() have been added to the docs.

    @aeros aeros closed this as completed Jul 7, 2019
    @terryjreedy
    Copy link
    Member

    Spinoff issues should be separate.

    @terryjreedy terryjreedy changed the title Docs: Method os.chdir() does not mention errors that can be raised Specify possible exceptions for os.chdir() Jul 7, 2019
    @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
    3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes docs Documentation in the Doc dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants