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

Better docs for importlib.resources #93610

Closed
encukou opened this issue Jun 8, 2022 · 22 comments
Closed

Better docs for importlib.resources #93610

encukou opened this issue Jun 8, 2022 · 22 comments
Labels
docs Documentation in the Doc dir topic-importlib

Comments

@encukou
Copy link
Member

encukou commented Jun 8, 2022

The current documentation of importlib.resources can be improved.
The main hindrance to usability are references to classes that are not linked correctly, leaving the docs very hard to follow for someone who doesn't already know the module. For example, Traversable in the docs of files is not a link.

@encukou encukou added docs Documentation in the Doc dir topic-importlib labels Jun 8, 2022
@encukou
Copy link
Member Author

encukou commented Jun 8, 2022

PR: #93611

@warsaw
Copy link
Member

warsaw commented Jun 10, 2022

Just ran across this one too! importlib.resources.rst links to importlib.resources.abc.Traversable but it should be importlib.abc.Traversable. PR coming for that.

@encukou were there other problems you found?

warsaw added a commit to warsaw/cpython that referenced this issue Jun 10, 2022
Closes python#93610

This also improves the error message when someone uses the older `make serve`
command of `Docs/Makefile`.
warsaw added a commit to warsaw/cpython that referenced this issue Jun 10, 2022
Closes python#89896

This also addresses python#93610 in that it fixes the link to
`importlib.abc.Traversable` too.
warsaw added a commit to warsaw/cpython that referenced this issue Jun 11, 2022
… a method

Closes python#89896

This also addresses python#93610 in that it fixes the link to
`importlib.abc.Traversable` too, for Python 3.10 and earlier.
@warsaw
Copy link
Member

warsaw commented Jun 11, 2022

I've opened two PRs which seem to fix the documentation problem, but I'm not actually sure this is the right thing to do. The deal is that in 3.11, Traversable, ResourceReader, and TraversableResources were moved to importlib.resources.abc, with this compatibility hack added to importlib.abc:

# for compatibility with Python 3.10
from .resources.abc import ResourceReader, Traversable, TraversableResources

However, the documentation for importlib.resources.abc is included in importlib.rst (inside the :mod:importlib.abc section):

.. include:: importlib.resources.abc.rst

which is why (I think), the doc cross references to importlib.resources.abc.Traversable don't work (without the above PRs). I don't see any mention of this reorg in the What's New in Python 3.11 docs, nor in the importlib.abc documentation, so I'm not sure what the intended public API for these ABC's is.

@jaraco can you weigh in?

@encukou
Copy link
Member Author

encukou commented Jun 13, 2022

Yes, that confused me as well, so I went with the implementation here – <class 'importlib.resources.abc.Traversable'>.

@encukou
Copy link
Member Author

encukou commented Jun 13, 2022

were there other problems you found?

I sent a PR with improvements, it's probably better to look there, but:

  • for migrating away from the deprecated functions, there's a link to importlib_resources docs, which says to look in the source of importlib_resources._legacy – which uses a private module for all of the implementation. (Perhaps with Traversable documentation being discoverable, this would be easier to use?)
  • the deprecated functions take up most of the page, and could be moved to a new “you don't need to read this” section
  • many references to types/functions could be links to the relevant docs, so they're more usable for people who don't already know them
  • it's pure reference; there's no hint about why you would use this and not just __file__

@jaraco
Copy link
Member

jaraco commented Jun 13, 2022

I don't see any mention of this reorg in the What's New in Python 3.11 docs, nor in the importlib.abc documentation, so I'm not sure what the intended public API for these ABC's is.

At the time of writing, I considered the ABCs to be primarily an implementation detail, maybe occasionally referenced in the source for clarity. I wasn't even sure there should be a concrete ABC to describe the interface, but as it developed, the value of the abcs became more apparent and started having references in the documentation.

My preference is for code to be self-documenting as much as possible, limiting the amount of duplication that has to happen in prose.

When I moved the module, I still considered the Traversable object to be primarily an implementation detail, although by moving it to the abcs module, I was preparing it to be supported first-class for type hints.

It looks like the Traversable class was moved in python/importlib_resources#87, quite some time ago. Oh, that's not when the content moved to its own module under resources. bpo-46118 is where that work happened. Perhaps this news entry could have been more explicit about the move of some interfaces from importlib.abc to importlib.resources.abc.

@encukou
Copy link
Member Author

encukou commented Jun 13, 2022

So, should it be documented as importlib.resources.abc.Traversable or importlib.abc.Traversable?

@warsaw
Copy link
Member

warsaw commented Jun 13, 2022

Yes, that confused me as well, so I went with the implementation here – <class 'importlib.resources.abc.Traversable'>.

Except that link doesn't work.

So, should it be documented as importlib.resources.abc.Traversable or importlib.abc.Traversable?

If the latter, my PR fixes that link. If the former, perhaps it shouldn't be :include:'d in importlib.abc docs?

@encukou
Copy link
Member Author

encukou commented Jun 14, 2022

If the former, my PR fixes the link, by moving importlib.resources.abc to its own page.

@warsaw
Copy link
Member

warsaw commented Jun 14, 2022

I'd really like for @jaraco to weigh in. I'm guessing he'd prefer to split it into importlib.resources.abc since it will be easier to keep the standalone backport in sync. OTOH, importlib.resources was added to Python in 3.7, so how much longer does importlib-resources still need to be actively maintained? It had an upload back in April, but I don't know what the roadmap is for that package.

If it was up to me (and maybe it's good that it's not!), I'd schedule importlib-resources for retirement and stop development on it. Then I would probably leave Traversable and such in importlib.abc, without the artificial distinction. But that's just me.

@warsaw
Copy link
Member

warsaw commented Jun 14, 2022

@encukou Depending on what @jaraco says, I'll happily review your branch and close mine if appropriate.

@encukou
Copy link
Member Author

encukou commented Jun 15, 2022

importlib.resources was added to Python in 3.7

Everything added in 3.7 is now deprecated. The current API is from 3.9.

@warsaw
Copy link
Member

warsaw commented Jun 15, 2022

Everything added in 3.7 is now deprecated. The current API is from 3.9.

I guess that's a valid reason for keeping the standalone package around, but at some point (maybe 3.9 EOL?) it's likely not worth it. Then again, that's a @jaraco decision.

@jaraco
Copy link
Member

jaraco commented Jun 16, 2022

Everything added in 3.7 is now deprecated. The current API is from 3.9.

I guess that's a valid reason for keeping the standalone package around, but at some point (maybe 3.9 EOL?) it's likely not worth it. Then again, that's a @jaraco decision.

I'd not say the current API is from 3.9. A major change landed in 3.9, but more changes landed in 3.10 and some have yet to land (for example, python/importlib_resources#250 was inspired by recent feedback). You can see in the compatibility matrix which versions of _resources map to which versions of CPython, and you can use the history to see what changes occurred between those versions. So users who want features from importlib_resources 5.1 or later will need to use Python 3.11 or importlib_resources.

If at some point, you want not to support backporting any behavior, we should make that decision, but the status quo is to develop the changes in the backport, prove their viability, then port them into the next CPython. Same is true for importlib.metadata.

I'd really like for @jaraco to weigh in. I'm guessing he'd prefer to split it into importlib.resources.abc since it will be easier to keep the standalone backport in sync. OTOH, importlib.resources was added to Python in 3.7, so how much longer does importlib-resources still need to be actively maintained? It had an upload back in April, but I don't know what the roadmap is for that package.

There's no roadmap per se. It's pretty stable, but if there's an improvement added in the latest version, it improves adoption to be able to make that feature available in the backport.

Yes, I'd prefer to keep abcs for resources in that package. Even in isolation, it's better to have abcs in the same package where they're used for logical encapsulation (for the same reason that you don't put abcs from collections.abc and importlib.abc in a top-level .abc).

That said, it also helps the backport immensely, since the code that exists in 'importlib_resources' has a direct mapping to the same module in importlib.resources.

If it was up to me (and maybe it's good that it's not!), I'd schedule importlib-resources for retirement and stop development on it. Then I would probably leave Traversable and such in importlib.abc, without the artificial distinction. But that's just me.

I would like to retire the backport too, but I also find it 10x easier to develop in the backport (massively better test runner, faster tests, no compiler needed, fast iteration, coverage testing, ...). Let's not make that decision an an "improve docs" bug. Maybe file a bug with importlib-resources?

For all the reasons above, let's use importlib.resources.abc. It's a better choice. I'll update the what's changed to include that move. Let's get the docs to match.

@encukou
Copy link
Member Author

encukou commented Jun 16, 2022

If at some point, you want not to support backporting any behavior, we should make that decision, but the status quo is to develop the changes in the backport, prove their viability, then port them into the next CPython. Same is true for importlib.metadata.

IMO, that's a good strategy. The downside is that since the docs aren't synced with the backport, they feel somewhat second-class – they're split between CPython docs (reference) and backport docs (guides), and references to the docs (or to source code) seem awkward.
I hope that doesn't read as too negative. I mean it as constructive criticism – pointing out what I see as weak spots so I can help make them better.
And I don't mean to imply you need to work on the docs – if it's a chore you'd rather delegate, just say so! IMO there are people who'd love to work on this but are somewhat wary of touching “someone else's” docs.

For all the reasons above, let's use importlib.resources.abc. It's a better choice. I'll update the what's changed to include that move. Let's get the docs to match.

@warsaw, do you want to review #93611?

@jaraco, what's the plan for having Traversable importable from importlib.abc? Is that temporary, or should it be documented and supported?

@jaraco
Copy link
Member

jaraco commented Jun 16, 2022

they feel somewhat second-class – they're split between CPython docs (reference) and backport docs (guides)

And this split is inconsistent across .metadata and .resources, because docs for .resources were developed primarily in CPython but docs for .metadata were developed primarily in the backport. I'm unsure which model is preferable, but I'd like to align them. I lean slightly toward the .resources model, where most of the docs (reference and guides) are in CPython and maintained there, with backport-specific details in the backport.

See python/importlib_resources#240 where I've invited others to contribute to the docs.

wary of touching “someone else's” docs

Ultimately, I want not to be a blocker or hindrance to contribution, but I do want to avoid increasing maintenance toil. I'd prefer to have a development partner who can share ownership of the design, and reserve drive-by contributions to those that are mostly uncontroversial.

@jaraco, what's the plan for having Traversable importable from importlib.abc? Is that temporary, or should it be documented and supported?

I'd say the presence in importlib.abc is deprecated. I'm unsure if it actually needs to go through a deprecation process, since it wasn't documented.

@encukou
Copy link
Member Author

encukou commented Jun 16, 2022

Ultimately, I want not to be a blocker or hindrance to contribution, but I do want to avoid increasing maintenance toil. I'd prefer to have a development partner who can share ownership of the design, and reserve drive-by contributions to those that are mostly uncontroversial.

What do you mean by sharing ownership of the design?

I'd say the presence in importlib.abc is deprecated. I'm unsure if it actually needs to go through a deprecation process, since it wasn't documented.

It turns out it is documented in 3.10.
It's importlib.resources.abc.Traversable that isn't documented (and that's the name mentioned in the function docs).

@jaraco
Copy link
Member

jaraco commented Jun 16, 2022

What do you mean by sharing ownership of the design?

I just mean I'm looking for someone who cares about the product and the design largely, someone invested enough to understand the motivations behind the existing implementation.

It turns out it is documented in 3.10.
It's importlib.resources.abc.Traversable that isn't documented (and that's the name mentioned in the function docs).

Okay. Then a deprecation process is probably in order. Let's get the docs to point to the preferred location (resources.abc) and worry about the deprecation separately.

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jul 25, 2022
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Co-authored-by: Stanley <46876382+slateny@users.noreply.github.com>
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
(cherry picked from commit ccd7c7a)

Co-authored-by: Petr Viktorin <encukou@gmail.com>
ambv added a commit that referenced this issue Jul 25, 2022
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Co-authored-by: Stanley <46876382+slateny@users.noreply.github.com>
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
ambv added a commit that referenced this issue Jul 25, 2022
(cherry picked from commit ccd7c7a)

Co-authored-by: Petr Viktorin <encukou@gmail.com>
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Co-authored-by: Stanley <46876382+slateny@users.noreply.github.com>
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
@jaraco
Copy link
Member

jaraco commented Nov 26, 2022

I appreciate all of work you've done, @encukou . Is there anything more you'd like to see here? I'm closing this as complete, but feel free to re-open or otherwise revive the conversation.

@jaraco jaraco closed this as completed Nov 26, 2022
@aiven-anton
Copy link

aiven-anton commented Dec 6, 2022

Hi, I was about to open a new issue but noticed this discussion seems somewhat recent.

Currently importlib.metadata.distributions (link) is not documented. Is this covered by the PRs that are already merged?

It seems like this full API reference from the importlib-metadata project should somehow make its way into the CPython docs?

For this specific case, is it safe to consider distributions public?

Please let me know if I should open a separate issue.

@jaraco
Copy link
Member

jaraco commented Dec 6, 2022

Currently importlib.metadata.distributions (link) is not documented.

This issue is mainly about importlib.resources, which is different from importlib.metadata, though I too get those mixed up sometimes, and this issue did touch on importlib.metadata.

It seems like this full API reference from the importlib-metadata project should somehow make its way into the CPython docs?

Unfortunately, last I checked, there's no support in CPython for generating the API docs from the source (where the behavior is documented). See python/importlib_metadata#119 for some work in this area. The solution at the time was for the CPython docs to link back to the API docs in importlib_metadata, which is still present.

For this specific case, is it safe to consider distributions public?

Yes. Any names exposed by __all__ can be relied upon with the highest confidence. Any non-private names (those without a leading _) may also be fair game, though I would advise against using implementation details like Sectioned. In the past, I've not marked these classes private, but to my chagrin, I've since learned that that's the recommendation.

If that didn't answer your questions, feel free to open a new issue.

@aiven-anton
Copy link

@jaraco Thanks for your very thorough response, and sorry for the mixup, this answers all my questions! I didn't notice the reference to the API reference when first reading the docs, now I see it's there, so all good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir topic-importlib
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants