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

importlib.abc.TraversableReader is not implemented #88361

Closed
FFY00 opened this issue May 20, 2021 · 6 comments
Closed

importlib.abc.TraversableReader is not implemented #88361

FFY00 opened this issue May 20, 2021 · 6 comments
Labels
3.9 only security fixes type-bug An unexpected behavior, bug, or error

Comments

@FFY00
Copy link
Member

FFY00 commented May 20, 2021

BPO 44195
Nosy @brettcannon, @jaraco, @miss-islington, @FFY00
PRs
  • bpo-44195: add importlib.abc.TraversableResources #26271
  • bpo-44196: document that Traversable's methods should raise FileNotFoundError #26272
  • bpo-44195: Use 'TraversableResources' in the docs to match the implementation. #26317
  • [3.10] bpo-44195: Use 'TraversableResources' in the docs to match the implementation. (GH-26317) #26334
  • [3.9] bpo-44195: Use 'TraversableResources' in the docs to match the implementation. (GH-26317) #26335
  • 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-07-30.02:14:55.057>
    created_at = <Date 2021-05-20.19:19:52.597>
    labels = ['type-bug', '3.9']
    title = 'importlib.abc.TraversableReader is not implemented'
    updated_at = <Date 2021-07-30.02:14:55.056>
    user = 'https://github.com/FFY00'

    bugs.python.org fields:

    activity = <Date 2021-07-30.02:14:55.056>
    actor = 'jaraco'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-07-30.02:14:55.057>
    closer = 'jaraco'
    components = []
    creation = <Date 2021-05-20.19:19:52.597>
    creator = 'FFY00'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 44195
    keywords = ['patch']
    message_count = 6.0
    messages = ['394052', '394197', '394204', '394257', '394258', '394259']
    nosy_count = 4.0
    nosy_names = ['brett.cannon', 'jaraco', 'miss-islington', 'FFY00']
    pr_nums = ['26271', '26272', '26317', '26334', '26335']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue44195'
    versions = ['Python 3.9']

    @FFY00
    Copy link
    Member Author

    FFY00 commented May 20, 2021

    7f7e706 introduced the files() api and documented a importlib.abc.TraversableReader protocol, but it did not implement it.

    This class is documented and marked as introduced in 3.9, but it's actually missing.

    @FFY00 FFY00 added 3.9 only security fixes type-bug An unexpected behavior, bug, or error labels May 20, 2021
    @jaraco
    Copy link
    Member

    jaraco commented May 23, 2021

    I don't believe a TraversableReader protocol was intended. Instead, the referenced change introduced the TraversableResources ABC. There's a typo in the docs. I created PR 26317 instead to correct the mistake. WDYT?

    @FFY00
    Copy link
    Member Author

    FFY00 commented May 23, 2021

    We do implement the protocol, and not TraversableResources, in some places, such as DegenerateFiles for eg. It seems to me that maybe that is an issue and we actually want to inherit from TraversableResources.

    The question here is, are people supposed to be implementing readers with just files(), or are they always expected to inherit TraversableResources?

    Regardless of the usefulness in code, please also consider type hinting. If people are expecting to be using this protocol, we should expose it.

    @jaraco
    Copy link
    Member

    jaraco commented May 24, 2021

    are people supposed to be implementing readers with just files(), or are they always expected to inherit TraversableResources?

    A resource provider could potentially implement only the files() method (what I think you're calling TraversableReader), but my expectation was that all providers would provide a Reader derived from TraversableResources in order to provide backward-compatibility for the ResourceReader interface. Long term, I'd expect to deprecate all but files() on TraversableResources.

    If a provider only implemented files() today and did not inherit from TraversableResources, they would still satisfy the files() API, but not the ResourceReader API (i.e. violate the expectation that Loader.get_resource_reader returns a ResourceReader).

    I decided not to present both TraversableReader and TraversableResources as separate classes because the latter was sufficient for all known cases.

    It seems to me that maybe that is an issue and we actually want [DegenerateFiles] to inherit from TraversableResources.

    Perhaps. What advantage would that have?

    Regardless of the usefulness in code, please also consider type hinting.

    Agreed, there are some places where type hints would drastically improve readability.

    If people are expecting to be using this protocol, we should expose it.

    My instinct is TraversableResources is the preferred protocol for now, although I think it's likely we'll want to separate out the TraversableReader when necessary. Let's plan to do that in importlib_resources first.

    @miss-islington
    Copy link
    Contributor

    New changeset d309bcc by Miss Islington (bot) in branch '3.10':
    bpo-44195: Use 'TraversableResources' in the docs to match the implementation. (GH-26317)
    d309bcc

    @miss-islington
    Copy link
    Contributor

    New changeset ab4da07 by Miss Islington (bot) in branch '3.9':
    [3.9] bpo-44195: Use 'TraversableResources' in the docs to match the implementation. (GH-26317) (GH-26335)
    ab4da07

    @jaraco jaraco closed this as completed Jul 30, 2021
    @jaraco jaraco closed this as completed Jul 30, 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.9 only security fixes type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants