Merge lp:~adiroiban/pydoctor/1282458-apilinks-anchor into lp:~mwhudson/pydoctor/dev

Proposed by Adi Roiban
Status: Needs review
Proposed branch: lp:~adiroiban/pydoctor/1282458-apilinks-anchor
Merge into: lp:~mwhudson/pydoctor/dev
Diff against target: 170 lines (+117/-5)
4 files modified
CONTRIBUTING.txt (+5/-0)
pydoctor/apilinks_sphinxext.py (+17/-5)
pydoctor/test/test_apilinks_sphinxext.py (+91/-0)
test-requirements.txt (+4/-0)
To merge this branch: bzr merge lp:~adiroiban/pydoctor/1282458-apilinks-anchor
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Pending
Review via email: mp+207392@code.launchpad.net

Description of the change

This is the branch which add anchor support for apilink Sphinx extension.

I have created CONTRIBUTING.txt file since as a first time contributor it
was not obvious that py.test is required together with 'py' packages.

I have created test-requirements.txt file to list all packages requried
for running the tests.

I have also added docutils since the following test was failing
`pydoctor/test/test_templatewriter.py:55: AssertionError`

Also my new test will fail without docutils.

I have moved apilinks_sphinxext.py inside pydoctor package to make it
easier to test.. but maybe it is a stupid change.

I am not sure how you want to distribute the exentsion. Maybe create a separate package.

bunch was added as a minimal mock tool.

flake8 was added as a minimum tool for keeping code formatting consistent.

I added an initial set of tests for happy case.

Let me know if more tests are required.

Let me know if something needs changes.

Thanks!

To post a comment you must log in.
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

Hi. Thanks for proposing this branch!

It would probably have been better to do the different things in different branches maybe?

I don't use py.test any more to run the tests, but generally use nose instead. I'm not super happy about either tbh, and would like to convert them all to something more standardly-pyUnitish but well.

I don't know how apilinks_sphinxext.py is used at all -- if it is still useful when part of the pydoctor package, then it makes sense for it to be there rather than at top level.

Revision history for this message
Adi Roiban (adiroiban) wrote :

I can split this review... but considering that this is the only branch in the review queue I don't know what splitting it will make things easier.

I use nose and I am happy with it. Much better than standard pyunit.

I tried to use, but test were failing so I did not dig deeper and just went to install py.test, since I saw that tests required 'py' package which was talking about pytest.

apilinks_sphinxext.py is used in Twisted, but is not used as part of pydoctor package, but instead there is a copy of this file.

Thanks!

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

Adi Roiban <email address hidden> writes:

> I can split this review... but considering that this is the only
> branch in the review queue I don't know what splitting it will make
> things easier.

Well I find it helps to talk about one thing at once but you make a fair
point.

> I use nose and I am happy with it. Much better than standard pyunit.

OK, so...

> I tried to use, but test were failing so I did not dig deeper and just
> went to install py.test, since I saw that tests required 'py' package
> which was talking about pytest.

Can you try again? I think I just deleted the last remnants of the py
imports.

> apilinks_sphinxext.py is used in Twisted, but is not used as part of
> pydoctor package, but instead there is a copy of this file.

Right, I think we should be moving towards only having one copy of the
file but I don't know what's involved in that.

Cheers,
mwh

Revision history for this message
Adi Roiban (adiroiban) wrote :

I tried nosetests on trunk and there are still errors: https://gist.github.com/adiroiban/9125421

I like nosetests and so I would like to use it instead of py.test. That all :)

I will ask over twisted mailing list and try to get some feedback about how apilinks_sphinxext.py should be distributed.

My initial but report/patch was on Twisted Trac and there I was asked to send patch here... feels like bad communication.

Thanks!

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

Adi Roiban <email address hidden> writes:

> I tried nosetests on trunk and there are still errors: https://gist.github.com/adiroiban/9125421

Argh, I think I've fixed the final final import of py...

> I like nosetests and so I would like to use it instead of py.test. That all :)

Good!

> I will ask over twisted mailing list and try to get some feedback about how apilinks_sphinxext.py should be distributed.

That'd be great.

> My initial but report/patch was on Twisted Trac and there I was asked
> to send patch here... feels like bad communication.

Well, if you'd just sent me the change to apilinks_sphinxext, it'd have
been merged by now. I'm sorry there was no CONTRIBUTING.txt and thanks
for trying to write one, but it's not surprising that it was hard for
you to guess what should be in there!

Cheers,
mwh

614. By Adi Roiban

Merge trunk.

615. By Adi Roiban

Update test requirements and contributing page.

Revision history for this message
Adi Roiban (adiroiban) wrote :

All good now. I have updated requirements. I will wait for feedback from Twisted developers. I am not in a hurry to have this merged.

While this code is in a single branch, I think that you can always pick some files from here, changed them as you like and commit them in trunk.

As long as you can just push a commit in trunk without a bug report and a review request, I feel that is unfair to ask contributors to spend extra effort for creating bugs and review requests for such simple things.
No hard feelings, just sincere feedback!

Thanks!

Unmerged revisions

615. By Adi Roiban

Update test requirements and contributing page.

614. By Adi Roiban

Merge trunk.

613. By Adi Roiban

Initial code for anchor support and tests.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'CONTRIBUTING.txt'
2--- CONTRIBUTING.txt 1970-01-01 00:00:00 +0000
3+++ CONTRIBUTING.txt 2014-02-21 09:01:52 +0000
4@@ -0,0 +1,5 @@
5+Install all dependencies with `pip install -r test-requirements.txt -e .`
6+
7+Run tests with `nosetests pydoctor`
8+
9+Do a minimum style check for your changes using flake8: `flake8 PATH/FILE.py`
10
11=== renamed file 'apilinks_sphinxext.py' => 'pydoctor/apilinks_sphinxext.py'
12--- apilinks_sphinxext.py 2014-01-30 01:32:51 +0000
13+++ pydoctor/apilinks_sphinxext.py 2014-02-21 09:01:52 +0000
14@@ -11,10 +11,13 @@
15 """
16
17
18-
19 def make_api_link(name, rawtext, text, lineno, inliner,
20 options={}, content=[]):
21+ """
22+ This is the standard signature for a docutils role function.
23
24+ Reference: http://docutils.sourceforge.net/docs/howto/rst-roles.html
25+ """
26 from docutils import nodes, utils
27
28 # quick, dirty, and ugly...
29@@ -25,13 +28,19 @@
30 else:
31 full_name = label = text
32
33+ anchor = ''
34+ if '#' in full_name:
35+ full_name, anchor = full_name.split('#', 1)
36+ # Add back the hash.
37+ anchor = '#' + anchor
38+
39 #get the base url for api links from the config file
40 env = inliner.document.settings.env
41- base_url = env.config.apilinks_base_url
42+ base_url = env.config.apilinks_base_url
43
44 # not really sufficient, but just testing...
45 # ...hmmm, maybe this is good enough after all
46- ref = ''.join((base_url, full_name, '.html'))
47+ ref = ''.join((base_url, full_name, '.html', anchor))
48
49 node = nodes.reference(rawtext, utils.unescape(label), refuri=ref,
50 **options)
51@@ -41,9 +50,12 @@
52 return nodes, sys_msgs
53
54
55-# setup function to register the extension
56-
57 def setup(app):
58+ """
59+ Standard function for registering the extension.
60+
61+ Will be called by Sphinx.
62+ """
63 app.add_config_value('apilinks_base_url',
64 'http://twistedmatrix.com/documents/current/api/',
65 'env')
66
67=== added file 'pydoctor/test/test_apilinks_sphinxext.py'
68--- pydoctor/test/test_apilinks_sphinxext.py 1970-01-01 00:00:00 +0000
69+++ pydoctor/test/test_apilinks_sphinxext.py 2014-02-21 09:01:52 +0000
70@@ -0,0 +1,91 @@
71+"""
72+Tests for Sphinx extentions which generated links to documentation created
73+with pydoctor.
74+"""
75+from bunch import Bunch
76+
77+from pydoctor import apilinks_sphinxext
78+
79+BASE_URL = 'http://example.com/api/'
80+
81+
82+class DummySphinxApp(object):
83+ """
84+ Simple mock of Sphinx app class to help with testing.
85+ """
86+ def __init__(self):
87+ self.config_value = {}
88+ self.role = {}
89+
90+ def add_config_value(self, name, default, rebuild):
91+ self.config_value[name] = {'default': default, 'rebuild': rebuild}
92+
93+ def add_role(self, name, role):
94+ self.role[name] = role
95+
96+
97+def test_setup():
98+ """
99+ It will expose the `apilinks_base_url` configuration option and register
100+ using `api` role.
101+ """
102+ sphinx_app = DummySphinxApp()
103+
104+ apilinks_sphinxext.setup(sphinx_app)
105+
106+ assert {
107+ 'default': 'http://twistedmatrix.com/documents/current/api/',
108+ 'rebuild': 'env',
109+ } == sphinx_app.config_value['apilinks_base_url']
110+
111+ assert apilinks_sphinxext.make_api_link is sphinx_app.role['api']
112+
113+
114+def execute_api_role(rawtext, text):
115+ """
116+ Execute the `api` role and return the single generated node.
117+ """
118+ # Mock the environment via the inliner.
119+ environemnt = Bunch(config=Bunch(apilinks_base_url=BASE_URL))
120+ inliner = Bunch(document=Bunch(settings=Bunch(env=environemnt)))
121+
122+ nodes, sys_msgs = apilinks_sphinxext.make_api_link(
123+ 'api', rawtext, text, 42, inliner, options={}, content=[])
124+
125+ # Should not report any messages.
126+ assert sys_msgs == []
127+ # Contains a single node.
128+ assert len(nodes) == 1
129+ return nodes[0]
130+
131+
132+def test_make_api_link_no_achor():
133+ """
134+ Will create a reference to html file.
135+ """
136+ result = execute_api_role(
137+ rawtext=':api:`package.module.ClassName <LabelName>`',
138+ text='package.module.ClassName <LabelName>')
139+
140+ assert 'reference' == result.tagname
141+ assert 'LabelName' == result.astext()
142+ assert [(
143+ 'refuri',
144+ 'http://example.com/api/package.module.ClassName.html',
145+ )] == result.attlist()
146+
147+
148+def test_make_api_link_with_achor():
149+ """
150+ Will create a reference to html file.
151+ """
152+ result = execute_api_role(
153+ rawtext=':api:`package.module.ClassName#methodName <LabelName>`',
154+ text='package.module.ClassName#methodName <LabelName>')
155+
156+ assert 'reference' == result.tagname
157+ assert 'LabelName' == result.astext()
158+ assert [(
159+ 'refuri',
160+ 'http://example.com/api/package.module.ClassName.html#methodName',
161+ )] == result.attlist()
162
163=== added file 'test-requirements.txt'
164--- test-requirements.txt 1970-01-01 00:00:00 +0000
165+++ test-requirements.txt 2014-02-21 09:01:52 +0000
166@@ -0,0 +1,4 @@
167+nose >= 1.3.0
168+docutils >= 0.11
169+bunch >= 1.0.1
170+flake8 >= 2.1.0

Subscribers

People subscribed via source and target branches

to all changes:
to status/vote changes: