Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Add python docstrings to boost:python bindings #29

Closed
artemp opened this Issue · 24 comments

2 participants

@artemp
Owner

Add missing doc strings to core objects e.g Style,Rule,Filter,Datasource etc.

@artemp
Owner

[springmeyer] I think this is really, really important.

I could help with this given a bit of guidance from those familiar with boost python syntax. Perhaps we could focus on one set of really good docstrings for say:

{{{
import mapnik
help(mapnik.Map)
}}}

To use as a guide to follow.

Thoughts?

@artemp
Owner

[springmeyer] The sample patch tries to do a few things I'd like feedback on:

1) Reorders the def and properties in the order the user sees them when they call help(mapnik.Layer), just to make it easier for the docstring writer to make sure all things are accounted for.
2) Implements the convention of 4 space indents (I'm not sure how the rest of mapnik C++ code works but I'll look around)
3) Adds docstrings to the init() function as well as class functions
4) Adds sample usage with a few comments

I'm hoping to understand the best approach with the Layer class as an example, so any further work can be systematic...

@artemp
Owner

[springmeyer] layer docstrings committed in r763
map docstrings committed in r767

@artemp
Owner

[springmeyer] r768 shows some tab fixes for the python docstrings in source:trunk/bindings/python/mapnik_map.cpp which I'll check out and try to match in the other files.

Today I also skimmed http://www.python.org/doc/essays/styleguide.html which looks like it will provide some further guidance on how to write appropriate docstrings

@artemp
Owner

[springmeyer] Just noticed that you can also add argument aliases for functions/methods defined in boost::python. This is great, and r1080 is an example for the Map class that now gives extra info when the function arguments are not correct:

{{{

mapnik.Map()
Traceback (most recent call last):
File "", line 1, in
Boost.Python.ArgumentError: Python argument types in
Map.init(Map)
did not match C++ signature:
init(object* Map Instance, int width, int height)
__init_
(_object* Map Instance, int width, int height, std::string srs)
}}}

@artemp
Owner

[springmeyer] more pressing issues at hand, so pushing this to 0.7.0

@artemp
Owner

[podolsir] I've created a couple of patches which provide docstrings for some classes. Due to me being a Boost::Python newbie, I started with the rather simple ones:

  • mapnik_color_docstrings.patch: Color
  • mapnik_stroke_docstrings.patch: Stroke, includes adding another constructor overload in mapnik_enumeration.cpp
  • mapnik_coord_env_projection.patch: Coord, Envelope and Projection (they belong all together)

The patches apply to current trunk and are all independent of each other.

I hope I could integrate the docs in an acceptable way. Feel free to criticize the docs and the way they're included, I'll improve them if needed.

@artemp
Owner

[springmeyer] podolsir,

Looks great overall, thanks very much for this contribution. I've applied the color patch in r1305 after making the 'args' conditional upon boost version and removing some mixed tabs/spaces - please see CodingStandards.

Also, perhaps of value to you I added the simple scripts I use to build the python api docs, so that you might preview the docstring rendering while you work on future files (r1307)

@artemp
Owner

[podolsir] Replying to [comment:16 springmeyer]:

Looks great overall, thanks very much for this contribution. I've applied the color patch in r1305 after making the 'args' conditional upon boost version and removing some mixed tabs/spaces - please see CodingStandards.
Dane, thanks for integrating the patch and the feedback. I'll update the other patches accordingly in the next days and upload them here.

Also, perhaps of value to you I added the simple scripts I use to build the python api docs, so that you might preview the docstring rendering while you work on future files (r1307)
Yes, that will be very helpful, too. Thank you!

(I'll set the patch_needs_improvement flag here, too, as it quite correctly reflects the situation ;))

@artemp
Owner

[podolsir] Improved patches uploaded, unsetting the flag.

@artemp
Owner

[springmeyer] Hmm, still were tabs in your patches. Also I've just noticed that 'args' is deprecated and we should be using 'arg', which is a boost::python class:

http://www.boost.org/doc/libs/1_40_0/libs/python/doc/v2/args.html

made a few fixes in r1310,r1311,r1312 you might also want to follow

@artemp
Owner

[podolsir] Replying to [comment:19 springmeyer]:

Thanks for applying (and fixing)! :)

Hmm, still were tabs in your patches.
Sorry for that. I actually checked it while fixing and before uploading, but probably not thoroughly/often enough :/ I'll look harder next time...

Also I've just noticed that 'args' is deprecated and we should be using 'arg', which is a boost::python class:
http://www.boost.org/doc/libs/1_40_0/libs/python/doc/v2/args.html
Oh, indeed. I'll adjust the other patch.

I assume we don't yet use the enum with docstring because it was first introduced in 1.35.0? Would it help if we do an #ifdef version check in the mapnik_enumeration() constructor and just ignore the second parameter if BOOST_VERSION <= 1.35?

@artemp
Owner

[springmeyer] Replying to [comment:20 podolsir]:

Replying to [comment:19 springmeyer]:

Thanks for applying (and fixing)! :)

No problem.

Also I've just noticed that 'args' is deprecated and we should be using 'arg', which is a boost::python class:
http://www.boost.org/doc/libs/1_40_0/libs/python/doc/v2/args.html
Oh, indeed. I'll adjust the other patch.

great.

I assume we don't yet use the enum with docstring because it was first introduced in 1.35.0? Would it help if we do an #ifdef version check in the mapnik_enumeration() constructor and just ignore the second parameter if BOOST_VERSION <= 1.35?

Ya, I was getting errors on a system with boost 1.34.1 so I commented until I could test more. I bet your hunch is right. If you provide a patch with that enum stuff wrapped in #ifdef against trunk then I'll test on my boost 1.34 system.

  • dane
@artemp
Owner

[podolsir] Replying to [comment:21 springmeyer]:

Dane,

If you provide a patch with that enum stuff wrapped in #ifdef against trunk then I'll test on my boost 1.34 system.
Here it is, tab free ;)

Another point: All the #ifdefs around args() and arg() and your recent changes (r1313 for) made me wonder in which version these classes were introduced. Now I've spent at least 15 minutes looking at boost::python documentation, changelog and actual revision logs in their SVN. Turns out this args()/arg() stuff is there since 2003 and was released as early as 1.31.0 and hasn't changed much since.

Before I shortly upgraded to 1.37 which I use now, I used 1.34 and mapnik compiled and worked all well (mapnik_map.cpp used args() for 6 months now without any ifdefs, see r1080). Maybe we don't need those #ifdefs around arg() after all. They sure don't make the code look prettier... Or am I missing something?

Igor

@artemp
Owner

[springmeyer] Igor,

Yes, I've also noticed that the docstrings support seems to be in the boost docs prior to 1.34, but I actually added the ifdef around the Map args immediately after the initial commit because of compile errors on 1.34. So, yes ideally we would avoid the #ifdefs, but I've not been able to compile with 1.34.1 without them. Perhaps something is just messed up on my system...

@artemp
Owner

[podolsir] I've just uploaded the fixed patch for Coord, Envelope and Projection which applies against r1315. I also expanded the class documentation of Coord and Envelope with the operator information and moved them to init.py wrapper classes where they are easier to write and maintain (as much less quoting as needed).

@artemp
Owner

[springmeyer] excellent job. applied unchanged in r1316. Thanks!

@artemp
Owner

[springmeyer] aha. it is that the 'self' or class instance argument is not supported pre boost 1.35.

So something like this fails pre 1.35:

{{{
.def(init((arg("self"),arg("color"),arg("width")) ...
}}}

with a compile error including:
{{{
boost::python::detail::error::more_keywords_than_init_arguments'
}}}

but this works fine:

{{{
.def(init((arg("color"),arg("width")) ...
}}}

using this pre 1.35 compatible method we end up with 'arg1' in the generated docstring instead of 'self' but that is better than adding ugly #ifdef's I figure.

If we ever drop support for 1.34 if the future we could then easily add that extra 'self' argument keyword.

r1317 changes this for all relevant docstrings.

@artemp
Owner

[podolsir] > using this pre 1.35 compatible method we end up with 'arg1' in the generated docstring instead of 'self' but that is better than adding ugly #ifdef's I figure.
Yeah, I agree. And it allows for the bigger and more important part of the docs to be also present when compiling against 1.34 (with current #ifdefs, 1.34 users would see arg1, arg2, ... for all arguments, not just "self"). While it would probably be possible to write some macro to circumvent it, I don't think it's worth it for now.

On to the next patch :) I was playing with the your epydoc scripts and noticed that there were way too many classes shown in the output (only Coord, Envelope, Feature and Projection). These seems to be the classes that have pure-Python wrappers defined in {{{init.py}}}; the files for the classes in the _mapnik module. In the generated docs in SVN they are explicitly under {{{mapnik._mapnik}}} module, which also isn't very nice as most users won't ever see _mapnik in their code. When I regenerate the docs with your scripts, even mapnik._mapnik doesn't show up.

Python's own pydoc tool shows the same behavior (here on Python 2.6.2 at least).

Exporting methods explicitly by defining {{{all}}} in {{{init.py}}} helps there as the classes are shown and referenced in all indices with pydoc and your epydoc scripts. It also has the nice effect of not polluting the caller's namespace with things like "BoostPythonMetaclass", "setdlopenflags" and so on if the user does {{{from mapnik import *}}} (they currently get all dumped there as evidenced by the output of the dir() function).

The attached export_members_explicitly.patch enumerates all API members in {{{ all}}}.

@artemp
Owner

[springmeyer] Replying to [comment:27 podolsir]:

On to the next patch :) I was playing with the your epydoc scripts and noticed that there were way too many classes shown in the output (only Coord, Envelope, Feature and Projection). These seems to be the classes that have pure-Python wrappers defined in {{{init.py}}}; the files for the classes in the _mapnik module. In the generated docs in SVN they are explicitly under {{{mapnik._mapnik}}} module, which also isn't very nice as most users won't ever see _mapnik in their code. When I regenerate the docs with your scripts, even mapnik._mapnik doesn't show up.

Exactly. This is one of the reasons for #400, but you've already made it farther along with that description! :)

Exporting methods explicitly by defining {{{all}}} in {{{init.py}}} helps there as the classes are shown and referenced in all indices with pydoc and your epydoc scripts. It also has the nice effect of not polluting the caller's namespace with things like "BoostPythonMetaclass", "setdlopenflags" and so on if the user does {{{from mapnik import *}}} (they currently get all dumped there as evidenced by the output of the dir() function).

The attached export_members_explicitly.patch enumerates all API members in {{{ all}}}.

Excellent. Applied in r1319.

Podolsir, would you like commit access to continue this work without me getting in the way? :)

@artemp
Owner

[springmeyer] r1320 finishes the stroke docstrings by adding the enum support. Thanks!

@artemp
Owner

[podolsir] > Excellent.
Glad I could help.

Podolsir, would you like commit access to continue this work without me getting in the way? :)
Dane, I don't think you've been getting in the way, in fact I experienced you as being rather very helpful so far :)))

Independent of that, commit access would be cool, of course, as there are a lot of classes without docstrings yet and it would be simpler to work for me (and probably for you, too). And now that I at last managed to setup building boost 1.34 and 1.37 in parallel without them getting in each other's way on my machine, the backwards compatibility problems like in mapnik_enumeration and args shouldn't happen anymore.

So, yes, I would like commit access :)

@springmeyer springmeyer closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.