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

Proposed by Adi Roiban on 2014-02-20
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 2014-02-20 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.
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.

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!

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

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!

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 on 2014-02-21

Merge trunk.

615. By Adi Roiban on 2014-02-21

Update test requirements and contributing page.

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 on 2014-02-21

Update test requirements and contributing page.

614. By Adi Roiban on 2014-02-21

Merge trunk.

613. By Adi Roiban on 2014-02-20

Initial code for anchor support and tests.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== added file 'CONTRIBUTING.txt'
--- CONTRIBUTING.txt 1970-01-01 00:00:00 +0000
+++ CONTRIBUTING.txt 2014-02-21 09:01:52 +0000
@@ -0,0 +1,5 @@
1Install all dependencies with `pip install -r test-requirements.txt -e .`
2
3Run tests with `nosetests pydoctor`
4
5Do a minimum style check for your changes using flake8: `flake8 PATH/FILE.py`
06
=== renamed file 'apilinks_sphinxext.py' => 'pydoctor/apilinks_sphinxext.py'
--- apilinks_sphinxext.py 2014-01-30 01:32:51 +0000
+++ pydoctor/apilinks_sphinxext.py 2014-02-21 09:01:52 +0000
@@ -11,10 +11,13 @@
11"""11"""
1212
1313
14
15def make_api_link(name, rawtext, text, lineno, inliner,14def make_api_link(name, rawtext, text, lineno, inliner,
16 options={}, content=[]):15 options={}, content=[]):
16 """
17 This is the standard signature for a docutils role function.
1718
19 Reference: http://docutils.sourceforge.net/docs/howto/rst-roles.html
20 """
18 from docutils import nodes, utils21 from docutils import nodes, utils
1922
20 # quick, dirty, and ugly...23 # quick, dirty, and ugly...
@@ -25,13 +28,19 @@
25 else:28 else:
26 full_name = label = text29 full_name = label = text
2730
31 anchor = ''
32 if '#' in full_name:
33 full_name, anchor = full_name.split('#', 1)
34 # Add back the hash.
35 anchor = '#' + anchor
36
28 #get the base url for api links from the config file37 #get the base url for api links from the config file
29 env = inliner.document.settings.env38 env = inliner.document.settings.env
30 base_url = env.config.apilinks_base_url39 base_url = env.config.apilinks_base_url
3140
32 # not really sufficient, but just testing...41 # not really sufficient, but just testing...
33 # ...hmmm, maybe this is good enough after all42 # ...hmmm, maybe this is good enough after all
34 ref = ''.join((base_url, full_name, '.html'))43 ref = ''.join((base_url, full_name, '.html', anchor))
3544
36 node = nodes.reference(rawtext, utils.unescape(label), refuri=ref,45 node = nodes.reference(rawtext, utils.unescape(label), refuri=ref,
37 **options)46 **options)
@@ -41,9 +50,12 @@
41 return nodes, sys_msgs50 return nodes, sys_msgs
4251
4352
44# setup function to register the extension
45
46def setup(app):53def setup(app):
54 """
55 Standard function for registering the extension.
56
57 Will be called by Sphinx.
58 """
47 app.add_config_value('apilinks_base_url',59 app.add_config_value('apilinks_base_url',
48 'http://twistedmatrix.com/documents/current/api/',60 'http://twistedmatrix.com/documents/current/api/',
49 'env')61 'env')
5062
=== added file 'pydoctor/test/test_apilinks_sphinxext.py'
--- pydoctor/test/test_apilinks_sphinxext.py 1970-01-01 00:00:00 +0000
+++ pydoctor/test/test_apilinks_sphinxext.py 2014-02-21 09:01:52 +0000
@@ -0,0 +1,91 @@
1"""
2Tests for Sphinx extentions which generated links to documentation created
3with pydoctor.
4"""
5from bunch import Bunch
6
7from pydoctor import apilinks_sphinxext
8
9BASE_URL = 'http://example.com/api/'
10
11
12class DummySphinxApp(object):
13 """
14 Simple mock of Sphinx app class to help with testing.
15 """
16 def __init__(self):
17 self.config_value = {}
18 self.role = {}
19
20 def add_config_value(self, name, default, rebuild):
21 self.config_value[name] = {'default': default, 'rebuild': rebuild}
22
23 def add_role(self, name, role):
24 self.role[name] = role
25
26
27def test_setup():
28 """
29 It will expose the `apilinks_base_url` configuration option and register
30 using `api` role.
31 """
32 sphinx_app = DummySphinxApp()
33
34 apilinks_sphinxext.setup(sphinx_app)
35
36 assert {
37 'default': 'http://twistedmatrix.com/documents/current/api/',
38 'rebuild': 'env',
39 } == sphinx_app.config_value['apilinks_base_url']
40
41 assert apilinks_sphinxext.make_api_link is sphinx_app.role['api']
42
43
44def execute_api_role(rawtext, text):
45 """
46 Execute the `api` role and return the single generated node.
47 """
48 # Mock the environment via the inliner.
49 environemnt = Bunch(config=Bunch(apilinks_base_url=BASE_URL))
50 inliner = Bunch(document=Bunch(settings=Bunch(env=environemnt)))
51
52 nodes, sys_msgs = apilinks_sphinxext.make_api_link(
53 'api', rawtext, text, 42, inliner, options={}, content=[])
54
55 # Should not report any messages.
56 assert sys_msgs == []
57 # Contains a single node.
58 assert len(nodes) == 1
59 return nodes[0]
60
61
62def test_make_api_link_no_achor():
63 """
64 Will create a reference to html file.
65 """
66 result = execute_api_role(
67 rawtext=':api:`package.module.ClassName <LabelName>`',
68 text='package.module.ClassName <LabelName>')
69
70 assert 'reference' == result.tagname
71 assert 'LabelName' == result.astext()
72 assert [(
73 'refuri',
74 'http://example.com/api/package.module.ClassName.html',
75 )] == result.attlist()
76
77
78def test_make_api_link_with_achor():
79 """
80 Will create a reference to html file.
81 """
82 result = execute_api_role(
83 rawtext=':api:`package.module.ClassName#methodName <LabelName>`',
84 text='package.module.ClassName#methodName <LabelName>')
85
86 assert 'reference' == result.tagname
87 assert 'LabelName' == result.astext()
88 assert [(
89 'refuri',
90 'http://example.com/api/package.module.ClassName.html#methodName',
91 )] == result.attlist()
092
=== added file 'test-requirements.txt'
--- test-requirements.txt 1970-01-01 00:00:00 +0000
+++ test-requirements.txt 2014-02-21 09:01:52 +0000
@@ -0,0 +1,4 @@
1nose >= 1.3.0
2docutils >= 0.11
3bunch >= 1.0.1
4flake8 >= 2.1.0

Subscribers

People subscribed via source and target branches

to all changes:
to status/vote changes: