Add support for file:// URLs for intersphinx #882
Conversation
There was a problem hiding this comment.
Hello @idclifford, many thanks for the PR!
I don’t quite understand how this will work since the file path does not carry the information regarding relative to what URL the links should be generated for a given inventory file. Some documentation regarding that new feature would be good, this can be added to docs/source/sphinx-integration.rst.
Also, it would be good to have some test cases that checks:
- the inventory file loading from command line options
- the actual links generated when we refer to an object that is defined in a inventory file
Somewhat a generic question, but, is it really worth it to introduce a new dependency just to load a file from disk? Are you using this to load files from nfs shares?
Many Thanks
|
Hello @idclifford, do you wish to continue working on this feature? If so, I'de recommend the following approach:
Tell me what you think, Thanks, |
|
Thanks for at least considering the new feature. I had not planned on more extensive developments, but I understand you're not happy with the very basic addition. |
|
It's not that I'm not happy, I'm really happy when new contributors emerges. It's just that I need to see this working, at least have a couple of unit tests. I cannot merge your contribution without tests. Thanks for your understanding, |
|
I have updated based on your suggestion. |
|
I have added some unit tests as requested. |
|
Hello @idclifford, many thanks for the adjustments! This is going in the right direction :) Here is an example test case that would check the generated links when we load an inventory from a file, The tests can be added to def test_generate_then_load_file(tmp_path: Path,
inv_writer_nolog: sphinx.SphinxInventoryWriter):
from pydoctor.test.test_astbuilder import fromText
from pydoctor.test.test_epydoc2stan import docstring2html
# Create a testing inventory file under tmp_path/objects.inv
src = '''
class C:
def __init__(self, a):
self.a = a
'''
system = fromText(src, modname='mylib').system
tmp_path.mkdir()
inv_writer_nolog.generate(system.rootobjects, tmp_path)
# Then load this inventory file in a new system, include links to elements
src = '''
from mylib import C
class Client:
"L{C}"
a: bytes
"L{C.a}"
'''
system2 = fromText(src, modname='myclient',
systemcls=lambda: model.System(
model.Options.from_args(
[f'--intersphinx-file={tmp_path / "objects.inv"}']))).system
Client_doc = docstring2html(system2.allobjects['myclient.Client'])
Client_a_doc = docstring2html(system2.allobjects['myclient.Client.a'])
assert Client_doc == '<p><a href="???"><p>'
assert Client_a_doc == '<p><a href="???"><p>' |
|
Documentation is still to come. |
| def test_fetchIntersphinxInventories_content() -> None: | ||
| @pytest.fixture(scope='module') | ||
| def tempDir(request: FixtureRequest, tmp_path_factory: TempPathFactory) -> Path: | ||
| name = request.module.__name__.split('.')[-1] | ||
| return tmp_path_factory.mktemp(f'{name}-cache') | ||
|
|
||
|
|
||
| def test_fetchIntersphinxInventories_content(tempDir:Path) -> None: | ||
| """ | ||
| Download and parse intersphinx inventories for each configured | ||
| intersphix. | ||
| """ | ||
|
|
||
| options = Options.defaults() | ||
| options.intersphinx = [ | ||
| 'http://sphinx/objects.inv', | ||
| 'file:///twisted/index.inv', | ||
| ] | ||
| options.intersphinx = ['http://sphinx/objects.inv'] | ||
| url_content = { | ||
| 'http://sphinx/objects.inv': zlib.compress( | ||
| b'sphinx.module py:module -1 sp.html -'), | ||
| 'file:///twisted/index.inv': zlib.compress( | ||
| b'twisted.package py:module -1 tm.html -'), | ||
| b'sphinx.module py:module -1 sp.html -') | ||
| } | ||
|
|
||
| root_dir = tempDir | ||
| path = root_dir / 'objects.inv' | ||
| with open(path, 'wb') as f: | ||
| f.write(zlib.compress(b'twisted.package py:module -1 tm.html -')) | ||
|
|
||
| with open(root_dir / 'tm.html', "w") as f: | ||
| pass | ||
|
|
||
| options.intersphinx_file = [path] | ||
|
|
There was a problem hiding this comment.
Please avoid modifying existing unit tests, create new ones that checks only what you are trying to check. Many thanks in advance.
| assert (root_dir / 'module1.html').samefile(inv_reader_nolog.getLink('some.module1')) | ||
| assert (root_dir / 'module2.html').samefile(inv_reader_nolog.getLink('other.module2')) |
There was a problem hiding this comment.
Interesting... so if I understand correctly, your generating links that do not include a http scheme but are paths to files directly. Can you tell me more about your usage of this feature ? are you serving all pydoctor generated pages for many projects under the same folder, so these links are all relative in practice ?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #882 +/- ##
=======================================
Coverage 93.11% 93.12%
=======================================
Files 47 47
Lines 8699 8709 +10
Branches 1606 1607 +1
=======================================
+ Hits 8100 8110 +10
Misses 337 337
Partials 262 262 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This is a relatively simple update to support file:// urls in intersphinx. If the python library requests-file is available, file support will be added to the requests session.