quanah / net-ldapapi

The Net::LDAPapi Perl Module uses the OpenLDAP and Mozilla C api's to directly access and manipulate an LDAP v2 or LDAP v3 server.
8 stars 6 forks source link

POD documentation inconsistencies #13

Closed phillipod closed 9 years ago

phillipod commented 9 years ago

Hi,

The following LDAPapi.pm functions:

have the following documentation signatures:

=item add DN ATTR SCTRLS CCTRLS
=item add_s DN ATTR SCTRLS CCTRLS
=item modify DN MOD
=item modify_s DN MOD

They have the following rearrange() patterns:

sub add { $self->rearrange(['DN', 'MOD', 'SCTRLS', 'CCTRLS'], @args); }
sub add_s { $self->rearrange(['DN', 'MOD', 'SCTRLS', 'CCTRLS'], @args); }
sub modify {$self->rearrange(['DN', 'MOD', 'SCTRLS', 'CCTRLS'], @args); }
sub modify_s { $self->rearrange(['DN', 'MOD', 'SCTRLS', 'CCTRLS'], @args); }

This impacts the usability of the documentation for the add() and add_s() functions, as the uppercased names seem to indicate the name for named parameter passing.

Changing the POD wouldn't break existing working code, but the API then becomes murky. Changing the code (to e.g., 'ATTR' for add() and add_s()) breaks code only currently working through knowledge of internals.

What're your thoughts?

Cheers, Phillip

quanah commented 9 years ago

Hm, this seems somewhat broken overall. add/add_s are part of the deprecated OpenLDAP API, and no longer available. The new fuctions are ldap_add_ext and ldap_add_ext_s, which both take dn, attrs, sctrls, cctrls. (I know the Net::LDAPapi code uses them behind the scenes). Overall, I'm fine with the following:

If we move to Net::LDAPapi version 3.1.x, we can change the API however we want. The original concept was to have parity with the underlying C API. Both Mozilla and OpenLDAP's C API's support add_ext(_s). I'd like to see the Net::LDAPapi code remove the deprecated functions, and then we can fix the function documentation to match the correct current C API functions.

phillipod commented 9 years ago

Sounds good to me.

I believe this has been captured sufficiently within #14 and #17 that this particular ticket can be closed off now.