Merge lp:~rvb/maas/maas-api-doc into lp:~maas-committers/maas/trunk

Proposed by Raphaël Badin
Status: Merged
Merged at revision: 29
Proposed branch: lp:~rvb/maas/maas-api-doc
Merge into: lp:~maas-committers/maas/trunk
Diff against target: 0 lines
To merge this branch: bzr merge lp:~rvb/maas/maas-api-doc
Reviewer Review Type Date Requested Status
Gavin Panella (community) Approve
Review via email: mp+89263@code.launchpad.net

Commit message

Api doc and sphinx doc.

Description of the change

This branch adds the api documentation to the code, along with a view /api/doc/. Note that I did not make it pretty because it's going to be the job of our designer.

It also sets up sphinx for the whole project.

To post a comment you must log in.
Revision history for this message
Raphaël Badin (rvb) wrote :

I think setting PISTON_IGNORE_DUPE_MODELS to True is fine because the handlers with similar models are hooked up to completely different urls.

Revision history for this message
Gavin Panella (allenap) wrote :

I like this :) I have a few comments, and I think you need to fix some
stuff, but nothing major.

[1]

+api-doc: src/maasserver/api.py
+ bin/django gen_rst_api_doc > docs/api.rst

This depends on bin/django too.

[2]

+html-doc: api-doc
+ cd docs; make html; cd ..

This should be written as:

html-doc: api-doc
 $(MAKE) -C docs html

In fact, it's probably best to model the actual dependencies (and just
call html-doc "doc"; it's an often expected name):

docs/api.rst: bin/django src/maasserver/api.py
 bin/django gen_rst_api_doc > docs/api.rst

doc: docs/api.rst
 $(MAKE) -C docs html

[3]

html-doc and api-doc - or just doc - need to be added to .PHONY.

[4]

+Welcome to the MaaS developer documentat...

Wrap this please :)

[5]

+PISTON_IGNORE_DUPE_MODELS = True

What problem is this solving? Add a comment perhaps?

[6]

+ """
+ Read a specific Node.
+ """

This is a lot of screen space for not a lot. Perhaps put this in a
single line? I quite like the way we do it in Launchpad, but I'm not
wedded to it.

[7]

+ <h2>MaaS server Api Documentation</h2>

API is a TLA, I think it should be written "API". Also, if
"Documentation" is capitalized then I think "server" ought to be too,
or vice-versa.

+ <table>
+ <thead>
+ <tr><td>Ressource</td><td>Description</td></tr>

s/Ress/Res/

[8]

+ messages.append(
+ "%s %s\n %s\n\n" % (
+ method.http_name, doc.resource_uri_template,
+ method.doc))

Does Piston make sure method.doc (and, to a lesser extent, the other
things that get interpolated) is suitable for inserting into an reST
doc? Is it assumed that we write the docs using reST? That's fine with
me, and we should add it to HACKING.txt.

review: Approve
Revision history for this message
Gavin Panella (allenap) wrote :

[9]

.bzrignore seems to be sorted. Keep it that way?

[10]

Neither clean nor distclean remove docs/_build.

Revision history for this message
Raphaël Badin (rvb) wrote :

> [1]
> [2]
[...]
> doc: docs/api.rst
> $(MAKE) -C docs html

Done.

> [3]
>
> html-doc and api-doc - or just doc - need to be added to .PHONY.

Okay, what it this .PHONY thing anyway?

> [4]
> Wrap this please :)

Done.

> [5]
>
> +PISTON_IGNORE_DUPE_MODELS = True
>
> What problem is this solving? Add a comment perhaps?

I confess I'm not really sure, I'll investigate some more but like I said, I think it's fine (because I had a quick glance at the code and because the api seems to work).

> [6]
> This is a lot of screen space for not a lot. Perhaps put this in a
> single line? I quite like the way we do it in Launchpad, but I'm not
> wedded to it.

Good point.

> [7]
>
> + <h2>MaaS server Api Documentation</h2>
>
> API is a TLA, I think it should be written "API". Also, if
> "Documentation" is capitalized then I think "server" ought to be too,
> or vice-versa.

Done.

> + <table>
> + <thead>
> + <tr><td>Ressource</td><td>Description</td></tr>
>
> s/Ress/Res/

Arg!

> [8]
>
> + messages.append(
> + "%s %s\n %s\n\n" % (
> + method.http_name, doc.resource_uri_template,
> + method.doc))
>
> Does Piston make sure method.doc (and, to a lesser extent, the other
> things that get interpolated) is suitable for inserting into an reST
> doc? Is it assumed that we write the docs using reST? That's fine with
> me, and we should add it to HACKING.txt.

http_name is fine, and so is resource_uri_template. You're right, we need say in HACKING.txt that the docs for api methods should use reST.

Revision history for this message
Raphaël Badin (rvb) wrote :

> [9]
>
> .bzrignore seems to be sorted. Keep it that way?

Indeed…

> [10]
>
> Neither clean nor distclean remove docs/_build.

Done.

Revision history for this message
Raphaël Badin (rvb) wrote :

> [1]
>
> +api-doc: src/maasserver/api.py
> + bin/django gen_rst_api_doc > docs/api.rst
>
> This depends on bin/django too.

Euh, on second thought, I'm not sure about this one, why do you think that?

Revision history for this message
Gavin Panella (allenap) wrote :

> > [3]
> >
> > html-doc and api-doc - or just doc - need to be added to .PHONY.
>
> Okay, what it this .PHONY thing anyway?

It means that it's not a real file, and make unconditionally runs its
commands when it's requested or depended upon by another target; make
revolves around real files, so it's best to tell it when something is
not and will never be a real file.

Revision history for this message
Gavin Panella (allenap) wrote :

> > [1]
> >
> > +api-doc: src/maasserver/api.py
> > + bin/django gen_rst_api_doc > docs/api.rst
> >
> > This depends on bin/django too.
>
> Euh, on second thought, I'm not sure about this one, why do you
> think that?

Because it runs bin/django. Try `make distclean && make api-doc` and
you'll see the problem.

Revision history for this message
Raphaël Badin (rvb) wrote :

> > > [1]
> > >
> > > +api-doc: src/maasserver/api.py
> > > + bin/django gen_rst_api_doc > docs/api.rst
> > >
> > > This depends on bin/django too.
> >
> > Euh, on second thought, I'm not sure about this one, why do you
> > think that?
>
> Because it runs bin/django. Try `make distclean && make api-doc` and
> you'll see the problem.

Right, I'm stouuupid.

Preview Diff

Empty