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

Document that importlib.abc.Traversable's methods should raise FileNotFoundError #88362

Closed
FFY00 opened this issue May 20, 2021 · 5 comments
Closed
Labels
3.10 only security fixes docs Documentation in the Doc dir

Comments

@FFY00
Copy link
Member

FFY00 commented May 20, 2021

BPO 44196
Nosy @brettcannon, @jaraco, @FFY00
PRs
  • bpo-44196: document that Traversable's methods should raise FileNotFoundError #26272
  • 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 = None
    closed_at = <Date 2021-05-23.17:26:13.600>
    created_at = <Date 2021-05-20.19:42:10.482>
    labels = ['3.10', 'docs']
    title = "Document that importlib.abc.Traversable's methods should raise FileNotFoundError"
    updated_at = <Date 2021-05-23.17:26:13.599>
    user = 'https://github.com/FFY00'

    bugs.python.org fields:

    activity = <Date 2021-05-23.17:26:13.599>
    actor = 'FFY00'
    assignee = 'docs@python'
    closed = True
    closed_date = <Date 2021-05-23.17:26:13.600>
    closer = 'FFY00'
    components = ['Documentation']
    creation = <Date 2021-05-20.19:42:10.482>
    creator = 'FFY00'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 44196
    keywords = ['patch']
    message_count = 5.0
    messages = ['394055', '394198', '394205', '394209', '394210']
    nosy_count = 4.0
    nosy_names = ['brett.cannon', 'jaraco', 'docs@python', 'FFY00']
    pr_nums = ['26272']
    priority = 'normal'
    resolution = 'rejected'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue44196'
    versions = ['Python 3.10']

    @FFY00
    Copy link
    Member Author

    FFY00 commented May 20, 2021

    ResourceReader used to have this requirement, Traversable does not. We cannot add a requirement as there might be users not doing this, but we can ask new users to do so.

    We should document that FileNotFoundError should be raised if a file is not found, but that it is not guaranteed that it will for all Traversable implementations for compatibility reasons.

    @FFY00 FFY00 added the 3.10 only security fixes label May 20, 2021
    @FFY00 FFY00 added docs Documentation in the Doc dir 3.10 only security fixes labels May 20, 2021
    @jaraco
    Copy link
    Member

    jaraco commented May 23, 2021

    After reviewing the PR, I'm not convinced the Traversable methods need any specification about when to raise FileNotFoundError (or other exceptions). Let's flesh out what conditions demand prescribed interfaces.

    @FFY00
    Copy link
    Member Author

    FFY00 commented May 23, 2021

    Okay.

    Having a better look at it, I think we should only add this specification to open().

    Traversable, by design, makes it totally plausible that open() cannot be performed. It could be a nonexistent path, the Traversable could represent object where open() does not make sense given the underlying implementation (it's something that cannot be read), etc.

    If Traversable makes it possible for open() to not work, it should give us a mechanism to handle such situations.
    If the only thing I know about an object is that it implements Traversable, how can I use open() in a reliable manner without having my code explode?

    This mechanism does not need to be FileNotFoundError. But, in my opinion, it is what probably makes the most sense in this case. Please let me know if you have a different/better idea.

    @jaraco
    Copy link
    Member

    jaraco commented May 23, 2021

    Having a look at the open function (https://docs.python.org/3/library/functions.html#open), it does not define which exceptions might be thrown. Similarly for pathlib.Path.open (https://docs.python.org/3/library/pathlib.html#pathlib.Path.open) and zipfile.Path.open (https://docs.python.org/3/library/zipfile.html#zipfile.Path.open).

    And since the canonical implementation of a Traversable resource is those two Path classes, it seems infeasible for the protocol to enforce a specific constraint on the available exceptions.

    I think the best this protocol can do is advertise that the underlying implementation may raise an Exception if the target indicated by self cannot be opened (allowing for an Exception where appropriate), but in my opinion, that's the default interface for any protocol (or function) unless otherwise specified.

    I don't want to explicitly call out FileNotFoundError or IsADirectoryError because those exceptions happen to be the most common. Consider for example the case where permission is not granted for read:

    $ pathlib.Path('./foo').open()
    PermissionError: [Errno 13] Permission denied: 'foo'
    

    That's yet another case where Traversable.open might raise something other than FileNotFoundError.

    In my opinion, unless the Traversable objects are expected to handle or transform exceptions, it's fine to simply delegate to the underlying implementations and allow any exceptions to bubble through.

    @FFY00
    Copy link
    Member Author

    FFY00 commented May 23, 2021

    That makes sense, I wish all these details were documented, but it is more than enough precedent to choose to not add such enforcements to the specification.

    @FFY00 FFY00 closed this as completed May 23, 2021
    @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.10 only security fixes docs Documentation in the Doc dir
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants