[Freeipa-devel] make-lint failures

[Alexander Bokovoy]

On Wed, 30 Nov 2011, Martin Kosek wrote:
> > The original LDAPObject.add_s() has modlist as non-optional argument:
> >     def add_s(self,dn,modlist):
> > 
> > I don't think it is wise to break that API assumption.
> > 
> > In all the cases above it should get .add_s(entry) replaced by 
> > .addEntry(entry).
> > 
> > The reason these failures are shown is because Martin reverted the 
> > earlier version of Sumit's patch that you mistakenly committed. Sumit 
> > has produced new patch already but there is one minor issue in it 
> > (another .add_s() -> .addEntry() replacement needs to be done).
> > 
> 
> I don't think this is the reason. My revert patch (Revert "Add DNS
> service records for Windows") just removed the lines that Rob pushed, it
> did not touch any add_s() call.
John's patch removed wrappers around original SimpleLDAPObject's 
methods that allowed to pass anything you want there without arguments 
checking. These wrappers are not visible to pylint on F17/Rawhide and 
essentially we are getting there same warnings with John's patch on 
F16 and earlier because with the patch pylint now sees proper 
signatures.

We need to review all .add_s() calls to ensure their correctness. 
.modify_s() are simpler because they always were used with two 
arguments (dn and list of modifications) but in case of .add_s() we 
were abusing it with Entry as a single argument and were relying on 
the magic of wrapper methods to do transformations.

John's patch gives you this:
    def add_s(self, dn, modlist):
        if isinstance(dn, Entry):
            return IPASimpleLDAPObject.add_s(self, dn.dn, dn.toTupleList())
        else:
            return IPASimpleLDAPObject.add_s(self, dn, modlist)

but I think it is semantically better to be explicit and use 
.addEntry() here, especially if you have already went and created 
Entry few lines before that. I would see these .add_s() methods as a 
convenience tool for API compliance if you need quickly convert some 
other LDAP-based Python code being merged to the framework.
 
> Even the Sumit's rebased patch
> (freeipa-sbose-0008-5-Add-DNS-service-records-for-Windows.patch) does
> not solve these pylint issues.
Because it didn't cover all the .add_s() calls. I asked Sumit to 
consider those cases as well.
-- 
/ Alexander Bokovoy