Opened 7 years ago

Closed 6 years ago

#983 closed task (complete)

Python wrappers for ACLs

Reported by: vorner Owned by: jinmei
Priority: medium Milestone: Sprint-20110802
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: ACL
Estimated Difficulty: 5.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

We need a wrapper for the loader, including named loading (#978, #982) and the ACL base class (#977). We don't actually need wrappers for any concrete ACL classes (like IP check), the code needs to call the check only.

Subtickets

Change History (22)

comment:1 Changed 6 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 5

comment:2 Changed 6 years ago by stephen

  • Milestone changed from Next-Sprint-Proposed to Sprint-20110712

comment:3 Changed 6 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to accepted

comment:4 Changed 6 years ago by jinmei

trac983 is ready for review.

Probably the most controversial point is the module (package)
organization. I chose to make it a package, not a single module,
consisting of init.py and some .so objects under the isc/acl
directly. I did it because it would be more consistent with the
underlying C++ module (it has separate namespace for DNS related
definitions). On the other hand, it may look the module (package)
unnecessarily complicated, especially because we currently only have
DNS specific classes (and it's not clear if and when we expand it).

Another possibly non-trivial point is the load_request_acl() function.
It corresponds to getRequestLoader().load(). I didn't see the need
for directly defining the singleton loader in the python binding,
so I only exposed the interface of the top level load() function this
way.

One other thing is the trick made in configure.ac. As commented
there, I needed a separate variable to install all .py and .so under
isc/acl/ directory.

The organization and design issues aside, the implementation itself
should be quite straightforward. It's not so different from other
common binding implementations. Also, the ported interfaces are
limited.

This may not need a changelog entry, but this is a proposed one
anyway:

269.	[func]		jinmei
	Added python bindings for ACLs using the DNS request as the
	context.  They are accessible via the isc.acl.dns module.
	(Trac #983, git TBD)

comment:5 Changed 6 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:6 Changed 6 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:7 follow-up: Changed 6 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

To address the design points first. I like it to be split into two parts, I don't know if it's better to be done this way or if more modules can be packed into one .so, but it is probably simpler this way anyway.

The thing about load_request_acl ‒ I'm little bit worried here. I expect we'll add a way to write custom checks sometime in future and I'd really like it to be possible to write them in python as well. Then we would need to expose the loader itself to register it and would need to change the interface. But maybe I worry too soon.

The code itself looks clean, with few small details:

  • I fixed a problem in Makefile ‒ it failed here, because dnsacl could not find an exception type from libacl. And I think the libacl should not depend on dns acl.
  • I don't really understand this comment, what does it mean?
            // Install module constants.  Note that we can release our own
            // references to these objects because we don't have corresponding
            // C++ variables.
    
  • In PyInit_acl, can some of the installToModule thing fail? Shouldn't it be checked for NULL? Or is this some kind of our thing that throws?
  • Why are the included files with documentation .cc? Shouldn't they be .h?
  • src/lib/python/isc/acl/dns.py seems to have the legal header twice.
  • Should this really be TypeError?
    PyErr_SetString(PyExc_TypeError,
                        "RequestACL cannot be directly constructed");
    
  • The functions that construct the ACLs take strings. I get it is the same as with the logging, but the logging is not really exposed, it is used from within the ModuleConfig? thing. This might be used externally, do you think it makes sense to accept the list argument directly (and call the conversion to string to be used from within the C++ code)?

With regards

comment:8 in reply to: ↑ 7 ; follow-up: Changed 6 years ago by jinmei

Replying to vorner:

To address the design points first. I like it to be split into two parts, I don't know if it's better to be done this way or if more modules can be packed into one .so, but it is probably simpler this way anyway.

By "split into two parts", do you mean (e.g.) having isc.acl.acl and
isc.acl.dns? BTW, as far as I know .so cannot have multiple python
modules. The relationship between modules and .so's (or more commonly
.py's) is one to one mappings. So, our choice is either

  • having multiple .so's for multiple modules
  • having a single (or smaller number of) module that contains much more classes

The thing about load_request_acl ‒ I'm little bit worried here. I expect we'll add a way to write custom checks sometime in future and I'd really like it to be possible to write them in python as well. Then we would need to expose the loader itself to register it and would need to change the interface. But maybe I worry too soon.

In fact, I wondered how the python part is expected to be extendable.
So, as a future possibility I have no problem with exposing the
register and introducing a mechanism to use python version checks as
we see the real need for this. But these would even be bigger than a
single task, so would certainly be beyond the scope of this ticket.

One thing we can do within this ticket would be to introduce a wrapper
for (Request)Loader which would currently only has the load() method.
That way, if and when we introduce more extensions the code for
loading() won't have to be modified. If you like I'm okay with doing
this in this ticket. Do you?

The code itself looks clean, with few small details:

  • I fixed a problem in Makefile ‒ it failed here, because dnsacl could not find an exception type from libacl. And I think the libacl should not depend on dns acl.

The latter one is okay (it was an error of mine). As for the former,
do you mean this one?

+dns_la_LIBADD += acl.la

If we do this, my linker complains:

*** Warning: Linking the shared library dns.la against the loadable module
*** acl.so is not portable!
*** Warning: lib acl.so is a module, not a shared library

(actually I saw that in my development, which is why I didn't do this)
and, the resulting code fails in tests in a strange way:

  File "/Users/jinmei/src/isc/git/bind10-983/src/lib/python/isc/acl/tests/dns_test.py", line 89, in test_construct
    self.assertRaises(Error, RequestContext, ('example.com', 5300))
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.1/lib/python3.1/unittest.py", line 589, in assertRaises
    callableObj(*args, **kwargs)
SystemError: NULL result without error in PyObject_Call

How exactly do you mean by "dnsacl could not find an exception type
from libacl"? Is that a link time error or run time error? If it's
the latter, I thought having init.py import isc.acl.acl would
resolve the missing symbol problems. Or maybe you need to explicitly
import isc.acl.acl when you import isc.acl.dns.

If it's a link time error, maybe we'll need some workaround, e.g.,
eliminating inter .so dependency completely (which might result in
mostly redundant definitions in each .so module though).

  • I don't really understand this comment, what does it mean?
            // Install module constants.  Note that we can release our own
            // references to these objects because we don't have corresponding
            // C++ variables.
    

Would this be better?

        // Install module constants.  Note that we can let Py_BuildValue
        // "steal" the references to these object (by specifying false to
        // installToModule), because, unlike the exception cases above,
        // we don't have corresponding C++ variables (see the note in
        // pycppwrapper_util for more details).
  • In PyInit_acl, can some of the installToModule thing fail? Shouldn't it be checked for NULL? Or is this some kind of our thing that throws?

installToModule will throw if it fails. More important in this
context, if po_XXXError is NULL PyObjectContainer?(po_XXXError) will
throw. In fact, that's the design principle of this framework - to
make the wrapper code concise (not requiring too much error handling
within the wrapper) and safer (minimize the risk of null pointer
dereference or leaking reference on failure). See pycppwrapper_util.h
for more details.

  • Why are the included files with documentation .cc? Shouldn't they be .h?

Because they contain definitions and cannot be shared by multiple
.cc's. I don't think including a .cc is so uncommon, but it only
matters within this wrapper implementation and is a kind of a
preference matter, so I wouldn't strongly push it.

  • src/lib/python/isc/acl/dns.py seems to have the legal header twice.

Ah, good catch, thanks. Removed.

  • Should this really be TypeError?
    PyErr_SetString(PyExc_TypeError,
                        "RequestACL cannot be directly constructed");
    

Probably not. Choosing a reasonable python exception in the wrapper
code is always a difficult question to me. In many cases none of the
standard python exceptions is really suitable. In this specific case,
I simply changed it to ACLError.

  • The functions that construct the ACLs take strings. I get it is the same as with the logging, but the logging is not really exposed, it is used from within the ModuleConfig? thing. This might be used externally, do you think it makes sense to accept the list argument directly (and call the conversion to string to be used from within the C++ code)?

Ah, okay. I thought applications would have ACL configurations in the
form of a json object (in which case it should be easy and trivial to
convert them to strings), but they are actually converted to a more
native form. So, yes, I think that makes sense. But I'm afraid it's
too low level for the C++ binding code to convert such complicated
python objects. I'd introduce a top layer python script that has a
hidden C++ backend like the standard socket module, i.e, defining
dns.py and _dns.so and having the former import the latter (it would
make it trickier to share the .py in the in-source mode and in the
installed version, though). Would you agree with that?

comment:9 Changed 6 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:10 in reply to: ↑ 8 ; follow-ups: Changed 6 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

By "split into two parts", do you mean (e.g.) having isc.acl.acl and

Yes, I mean this.

isc.acl.dns? BTW, as far as I know .so cannot have multiple python
modules. The relationship between modules and .so's (or more commonly
.py's) is one to one mappings. So, our choice is either

It is true for .py files, but .so files have some more powerful tools it seems (at last the API reference suggests so), I think it could be possible to create a module object „by hand“ and plug it into another module or something. I didn't think about it much and I'm not sure it is really possible. But as I said previously, I think having multiple .so files is fine and is definitely much easier, so let's leave it this way.

The thing about load_request_acl ‒ I'm little bit worried here. I expect we'll add a way to write custom checks sometime in future and I'd really like it to be possible to write them in python as well. Then we would need to expose the loader itself to register it and would need to change the interface. But maybe I worry too soon.

In fact, I wondered how the python part is expected to be extendable.
So, as a future possibility I have no problem with exposing the
register and introducing a mechanism to use python version checks as
we see the real need for this. But these would even be bigger than a
single task, so would certainly be beyond the scope of this ticket.

Yes, agreed here.

One thing we can do within this ticket would be to introduce a wrapper
for (Request)Loader which would currently only has the load() method.
That way, if and when we introduce more extensions the code for
loading() won't have to be modified. If you like I'm okay with doing
this in this ticket. Do you?

That was what I originally thought, so it would be nice to have the limited class wrapper, where we can add the register in future.

The code itself looks clean, with few small details:

  • I fixed a problem in Makefile ‒ it failed here, because dnsacl could not find an exception type from libacl. And I think the libacl should not depend on dns acl.

The latter one is okay (it was an error of mine). As for the former,
do you mean this one?

+dns_la_LIBADD += acl.la

Yes, this solves my problem. If it isn't there, it fails with this (during tests):

Running test: dns_test.py
Traceback (most recent call last):
  File "/home/vorner/work/bind10/src/lib/python/isc/acl/tests/dns_test.py", line 19, in <module>
    from isc.acl.dns import *
  File "/home/vorner/work/bind10/src/lib/python/isc/acl/dns.py", line 52, in <module>
    from dns import *
ImportError: /home/vorner/work/bind10/src/lib/python/isc/acl/.libs/dns.so: undefined symbol: _ZN3isc3acl6python14po_LoaderErrorE
make[7]: *** [check-local] Error 1

I don't know if having the library loaded suffices here. I think my linker looks only into libraries that are noted as dependencies. Don't you load the library already before? I think the test imports the acl.acl module first. What do you suggest I try?

Would this be better?

        // Install module constants.  Note that we can let Py_BuildValue
        // "steal" the references to these object (by specifying false to
        // installToModule), because, unlike the exception cases above,
        // we don't have corresponding C++ variables (see the note in
        // pycppwrapper_util for more details).

Yes, probably.

  • In PyInit_acl, can some of the installToModule thing fail? Shouldn't it be checked for NULL? Or is this some kind of our thing that throws?

installToModule will throw if it fails. More important in this
context, if po_XXXError is NULL PyObjectContainer?(po_XXXError) will
throw. In fact, that's the design principle of this framework - to
make the wrapper code concise (not requiring too much error handling
within the wrapper) and safer (minimize the risk of null pointer
dereference or leaking reference on failure). See pycppwrapper_util.h
for more details.

OK, I see. The naming conventions looked a lot like the original python API, so I thought it belongs there.

  • Why are the included files with documentation .cc? Shouldn't they be .h?

Because they contain definitions and cannot be shared by multiple
.cc's. I don't think including a .cc is so uncommon, but it only
matters within this wrapper implementation and is a kind of a
preference matter, so I wouldn't strongly push it.

OK, it is just unusual to me I guess. No problem there.

PyErr_SetString(PyExc_TypeError,
                    "RequestACL cannot be directly constructed");

Probably not. Choosing a reasonable python exception in the wrapper
code is always a difficult question to me. In many cases none of the
standard python exceptions is really suitable. In this specific case,

I updated a test to match the exception.

  • The functions that construct the ACLs take strings. I get it is the same as with the logging, but the logging is not really exposed, it is used from within the ModuleConfig? thing. This might be used externally, do you think it makes sense to accept the list argument directly (and call the conversion to string to be used from within the C++ code)?

Ah, okay. I thought applications would have ACL configurations in the
form of a json object (in which case it should be easy and trivial to
convert them to strings), but they are actually converted to a more
native form. So, yes, I think that makes sense. But I'm afraid it's
too low level for the C++ binding code to convert such complicated
python objects. I'd introduce a top layer python script that has a
hidden C++ backend like the standard socket module, i.e, defining
dns.py and _dns.so and having the former import the latter (it would
make it trickier to share the .py in the in-source mode and in the
installed version, though). Would you agree with that?

Well, I didn't really mean writing C++ code to do the conversion. My idea was to take the object passed as a parameter, somehow load the json.dumps function (which is the python function used to convert to string) and call it with PyObject_CallObject, so effectively using the python function from C++ code. It should even handle parameter parsing.

Having a python wrapper sounds reasonably as well, but it looks more tricky to me.

Anyway, it probably isn't so much a problem, so if the solutions both look complicated, I'm OK with the current interface ‒ I just thought it could be more convenient.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 6 years ago by jinmei

Replying to vorner:

The latter one is okay (it was an error of mine). As for the former,
do you mean this one?

+dns_la_LIBADD += acl.la

Yes, this solves my problem. If it isn't there, it fails with this (during tests):

Running test: dns_test.py
Traceback (most recent call last):
  File "/home/vorner/work/bind10/src/lib/python/isc/acl/tests/dns_test.py", line 19, in <module>
    from isc.acl.dns import *
  File "/home/vorner/work/bind10/src/lib/python/isc/acl/dns.py", line 52, in <module>
    from dns import *
ImportError: /home/vorner/work/bind10/src/lib/python/isc/acl/.libs/dns.so: undefined symbol: _ZN3isc3acl6python14po_LoaderErrorE
make[7]: *** [check-local] Error 1

I don't know if having the library loaded suffices here. I think my linker looks only into libraries that are noted as dependencies. Don't you load the library already before? I think the test imports the acl.acl module first. What do you suggest I try?

Hmm, this is strange. dns_test.py explicitly import isc.acl.acl
before line 19 that triggered the exception above:

from isc.acl.acl import LoaderError, Error, ACCEPT, REJECT, DROP
from isc.acl.dns import *

which should involve loading .libs/acl.so, which defines
isc::acl::python::po_LoaderError.

Right now I have no idea why this happened or how we can work around
it (without breaking the MacOS build). I'll try to reproduce it on
bind10.isc.org.

comment:12 in reply to: ↑ 11 Changed 6 years ago by jinmei

Replying to jinmei:

I don't know if having the library loaded suffices here. I think my linker looks only into libraries that are noted as dependencies. Don't you load the library already before? I think the test imports the acl.acl module first. What do you suggest I try?

Hmm, this is strange. dns_test.py explicitly import isc.acl.acl
before line 19 that triggered the exception above:

Okay, after some more research I realized that sharing symbols
between multiple loadable modules is very tricky, almost infeasible
if we want to be portable.

So I've developed a different workaround and committed it to the
branch. Could you try and review it?

(This specific issue took much more time than I expected, so I've not
addressed other issues yet).

BTW, while working on this, I also considered an option of
consolidating multiple modules into a single loadable module (.so).
And, unless I miss something, it seems very difficult if not
impossible. It would be very close to re-implementing the import
mechanism of the Python interpreter, which would be very complicated
and contain subtle corner cases, and I'm not even sure if we can
reasonably achieve that only using the exposed API.

comment:13 Changed 6 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:14 Changed 6 years ago by vorner

  • Owner changed from vorner to jinmei

I like this way, it looks elegant and it could possibly save a lot of trouble :-).

Just two details. The dns.h is not mentioned in Makefile.am, and there seems to be a double negative in „and there shouldn't be no operation“.

Would you have a look at the other issues, please?

Thank you

comment:15 in reply to: ↑ 10 ; follow-up: Changed 6 years ago by jinmei

I believe I've addressed all open issues.

Replying to vorner:

One thing we can do within this ticket would be to introduce a wrapper
for (Request)Loader which would currently only has the load() method.
That way, if and when we introduce more extensions the code for
loading() won't have to be modified. If you like I'm okay with doing
this in this ticket. Do you?

That was what I originally thought, so it would be nice to have the limited class wrapper, where we can add the register in future.

Done. The module now has RequestLoader? class.

Would this be better?

        // Install module constants.  Note that we can let Py_BuildValue
        // "steal" the references to these object (by specifying false to
        // installToModule), because, unlike the exception cases above,
        // we don't have corresponding C++ variables (see the note in
        // pycppwrapper_util for more details).

Yes, probably.

Replaced the comment with this.

  • The functions that construct the ACLs take strings. I get it is the same as with the logging, but the logging is not really exposed, it is used from within the ModuleConfig? thing. This might be used externally, do you think it makes sense to accept the list argument directly (and call the conversion to string to be used from within the C++ code)?

[...]

Well, I didn't really mean writing C++ code to do the conversion. My idea was to take the object passed as a parameter, somehow load the json.dumps function (which is the python function used to convert to string) and call it with PyObject_CallObject, so effectively using the python function from C++ code. It should even handle parameter parsing.

Having a python wrapper sounds reasonably as well, but it looks more tricky to me.

Anyway, it probably isn't so much a problem, so if the solutions both look complicated, I'm OK with the current interface ‒ I just thought it could be more convenient.

I thought this would be on borderline in that we'd probably want to
keep the bindings as straightforward as possible. But as I tried it I
found it did not require too much of new code in this specific case,
so I implemented it anyway. I'm still not sure if we want to
including this portion or not, so I'm okay either with or without it.
Note also that I've updated pycppwrapper_util.h to help implement this
feature (it was not absolutely necessary, but I wanted to keep the
code concise and still safer with the help of the wrapper interfaces).

And,

Just two details. The dns.h is not mentioned in Makefile.am, and there seems to be a double negative in „and there shouldn't be no operation“.

dns.h should have been in Makefile.am when you made this comment...

dns_la_SOURCES = dns.h dns.cc dns_requestacl_python.h dns_requestacl_python.cc

I've fixed the latter one. Thanks for pointing it out.

comment:16 Changed 6 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:17 in reply to: ↑ 15 ; follow-up: Changed 6 years ago by vorner

  • Owner changed from vorner to jinmei

Replying to jinmei:

  • The functions that construct the ACLs take strings. I get it is the same as with the logging, but the logging is not really exposed, it is used from within the ModuleConfig? thing. This might be used externally, do you think it makes sense to accept the list argument directly (and call the conversion to string to be used from within the C++ code)?

[...]

Well, I didn't really mean writing C++ code to do the conversion. My idea was to take the object passed as a parameter, somehow load the json.dumps function (which is the python function used to convert to string) and call it with PyObject_CallObject, so effectively using the python function from C++ code. It should even handle parameter parsing.

Having a python wrapper sounds reasonably as well, but it looks more tricky to me.

Anyway, it probably isn't so much a problem, so if the solutions both look complicated, I'm OK with the current interface ‒ I just thought it could be more convenient.

I thought this would be on borderline in that we'd probably want to
keep the bindings as straightforward as possible. But as I tried it I
found it did not require too much of new code in this specific case,
so I implemented it anyway. I'm still not sure if we want to
including this portion or not, so I'm okay either with or without it.
Note also that I've updated pycppwrapper_util.h to help implement this
feature (it was not absolutely necessary, but I wanted to keep the
code concise and still safer with the help of the wrapper interfaces).

I think, when it is already written and is quite short, we might want to include it. It will be more convenient and more the python way.

But, the PyImport_AddModule ‒ it does not ensure the module is actually loaded into the interpretter (if somethind didn't load it before, it returns an empty module). Can we expect it is already loaded, as in the case of isc.acl.acl?

The documentation for the loader is taken from the C++ one and it speaks about loading the checks. There's no function and no binding for the checks currently, should that part be removed?

Thank you

comment:18 in reply to: ↑ 17 ; follow-up: Changed 6 years ago by jinmei

Replying to vorner:

I think, when it is already written and is quite short, we might want to include it. It will be more convenient and more the python way.

Okay.

But, the PyImport_AddModule ‒ it does not ensure the module is actually loaded into the interpretter (if somethind didn't load it before, it returns an empty module). Can we expect it is already loaded, as in the case of isc.acl.acl?

Ah, good point. Since the tests passed even if I didn't import json
explicitly, I blindly thought it was somehow imported as part of the
core system or something (but simply invisible until the script does
'import json'). The fact is that the json module is imported via
the isc.init.py, which imports isc.cc, whose init.py imports
cc.message, which imports json.

So, it's actually true that we could (normally) rely on the json
module has been imported by the time the acl.dns module is used, but
after figuring this out I now think it's probably too implicit and
fragile. In fact, a buggy or evil python script could explicitly
delete sys.modulejson? after importing isc.acl and before importing
isc.acl.dns, in which case the assumption isn't met.

So I revised the code further: First, isc.acl.init.py now
explicitly imports json (just to be explicit; as noted above, this is
actually a redundant operation in normal cases). Second, I moved the
access code to json.dumps() from load() to the class initialization
function. This way, we could defend us against some time of evil
behavior such as deleting sys.modulejson? after importing the dns
module.

The documentation for the loader is taken from the C++ one and it speaks about loading the checks. There's no function and no binding for the checks currently, should that part be removed?

It at least notes create/registration (if you mean it by 'loading the
checks') is not available in the python version:

To allow any kind of checks to exist in the application, creators are\n\
registered for the names of the checks (this feature is not yet\n\
available for the python API).\n\

Basically, I wanted to keep the pydoc as close to the original C++ doc
as possible because the latter would also be modified in future, and
we want to incorporate the changes to pydoc. To make the merge easier
it would be helpful if the two versions are not so different.

And, in this case, according to you there's some plan (maybe at a
lower priority though) to add this feature to the python version, and
if so, I'd keep these parts with a simple note that it's not currently
available.

At the moment I didn't touch the documentation string. If you still
want to remove unimplemented part, could you be more specific about
exactly which part should better be removed?

comment:19 Changed 6 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:20 in reply to: ↑ 18 ; follow-up: Changed 6 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

So I revised the code further: First, isc.acl.init.py now
explicitly imports json (just to be explicit; as noted above, this is
actually a redundant operation in normal cases). Second, I moved the
access code to json.dumps() from load() to the class initialization
function. This way, we could defend us against some time of evil
behavior such as deleting sys.modulejson? after importing the dns
module.

OK, this looks good.

The documentation for the loader is taken from the C++ one and it speaks about loading the checks. There's no function and no binding for the checks currently, should that part be removed?

It at least notes create/registration (if you mean it by 'loading the
checks') is not available in the python version:

To allow any kind of checks to exist in the application, creators are\n\
registered for the names of the checks (this feature is not yet\n\
available for the python API).\n\

I was worried about this paragraph, which is not true about the python version:

The class can be used to load the checks only. This is supposed to be\n\
used by compound checks to create the subexpressions.\n\

So either removing it or noting that this functionality is not wrapped yet could be helpful.

But either way, I think this is small enough change, so it can be merged (with or without this change).

Thank you

comment:21 in reply to: ↑ 20 Changed 6 years ago by jinmei

Replying to vorner:

I was worried about this paragraph, which is not true about the python version:

The class can be used to load the checks only. This is supposed to be\n\
used by compound checks to create the subexpressions.\n\

So either removing it or noting that this functionality is not wrapped yet could be helpful.

Okay, removed it for now, and added an explicit note as a comment that
there are differences between the C++ version and pydoc.

I'm now going to merge it to master and close the ticket.

Thanks for review.

comment:22 Changed 6 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.