Allow uri and base to be taken from system/user LDAP configuration #164
Conversation
|
A check failed because of a timeout; I restarted it. I'm not the best person to review the feature, but if no-one else chimes in, I'll look at it next week. |
Codecov Report
@@ Coverage Diff @@
## master #164 +/- ##
==========================================
+ Coverage 70.22% 70.22% +<.01%
==========================================
Files 49 49
Lines 4816 4746 -70
Branches 810 821 +11
==========================================
- Hits 3382 3333 -49
+ Misses 1101 1067 -34
- Partials 333 346 +13
Continue to review full report at Codecov.
|
Modules/LDAPObject.c
Outdated
| @@ -1162,7 +1162,7 @@ l_ldap_search_ext( LDAPObject* self, PyObject* args ) | |||
| int msgid; | |||
| int ldaperror; | |||
|
|
|||
| if (!PyArg_ParseTuple( args, "sis|OiOOdi", | |||
| if (!PyArg_ParseTuple( args, "zis|OiOOdi", | |||
There was a problem hiding this comment.
Just move the | to the left and initialize the targets with NULL/0.
There was a problem hiding this comment.
I don't think that's correct - as it stands, filter is stil required in this location (also, z expresses exactly what is required, doesn't it?).
There was a problem hiding this comment.
We can make all arguments optional, too. z is correct to accept None and map it to NULL string.
There was a problem hiding this comment.
This is only called from python-ldap Python code and that will already pass the right arguments in (with a default), so the separator can stay where it is?
| @@ -748,13 +748,13 @@ def result4(self,msgid=ldap.RES_ANY,all=1,timeout=None,add_ctrls=0,add_intermedi | |||
| resp_data = self._bytesify_results(resp_data, with_ctrls=add_ctrls) | |||
| return resp_type, resp_data, resp_msgid, decoded_resp_ctrls, resp_name, resp_value | |||
|
|
|||
| def search_ext(self,base,scope,filterstr=None,attrlist=None,attrsonly=0,serverctrls=None,clientctrls=None,timeout=-1,sizelimit=0): | |||
| def search_ext(self,base=None,scope=ldap.SCOPE_SUBTREE,filterstr=None,attrlist=None,attrsonly=0,serverctrls=None,clientctrls=None,timeout=-1,sizelimit=0): | |||
There was a problem hiding this comment.
Why did you decide to use subtree here?
There was a problem hiding this comment.
It seems the more general (for searches) and is more common in examples.
There was a problem hiding this comment.
The chief example of an interface with a default for scope is ldapsearch and it picks -s sub, so this is being consistent.
encukou
force-pushed
the
default-parameters
branch
from
9fc20df
to
331e03c
Compare
|
Rebased on current master, as C code was recently re-formatted. |
encukou
force-pushed
the
default-parameters
branch
from
331e03c
to
d3c06bc
Compare
The aim is to reduce the number of parameters that must be passed to
various functions to the minimal set when LDAP is correctly configured
in the environment. In my case, ldap.conf contains at least the URI
and base, and is deployed externally to relevant machines. Duplicating
the configuration in my code would be both inconvenient and more
fragile. I believe this is the case in many common setups.
I think the ideal situation would actually be to re-order some of the
method parameters. People using defaults from the system configuraiton
probably want to only specify e.g. the filter. Putting it first (with
uri/base towards the end), would allow removing the keyword in
calls. However, that is a minor inconvenience versus breaking backward
compatibility.
This is a prototype pull request - in particular:
have the impression that testing could be achieved via environment
variables and would be willing to add tests).
urlfetchand am not 100% that I have caughtall locations that could be defaulted.