[Freeipa-devel] DN patch and documentation

[Petr Viktorin]

On 07/26/2012 10:11 PM, John Dennis wrote:
> On 07/17/2012 06:47 PM, John Dennis wrote:
>>> ipapython/dn.py:
>>>        in docstring:  DN(arg0, ..., locked=False, first_key_match=True)
>>>        followed by:  def __init__(self, *args, **kwds):
>>>        and:  kwds.get('first_key_match', True)
>>>
>>> I don't see the reason for this. Just write `def __init__(self, *args,
>>> locked=False, first_key_match=True)` and put a proper summary in the
>>> first line of the docstring. Same in AVA & RDN.
>>
>> A valid point. I think I was trying to be too flexible/extensible.
>
> Sorry, I was unable to make the suggested change. I tried and it
> refreshed my memory as to why it was coded the way it was.
>
> Python considers it a syntax error if keyword arguments follow an arg
> list reference. e.g.
>
>  >>> def foo(*args, bar=None):
>    File "<stdin>", line 1
>      def foo(*args, bar=None):
>                       ^
> SyntaxError: invalid syntax
>
> I'm not sure why, but that's why the methods are coded as
>
> def foo(*args, **kwds):
>
> If you have a suggestion as to how to use named parameters in this
> context let me know or maybe explain why it's an illegal construct (I'm
> sure it's in the language reference documentation somewhere, I just
> didn't take the time to research it).
>

You're right, I forgot this particular Python wart. It's only fixed in 
Python 3.


Looking at the code further, I recommend just getting rid of 
first_key_match.

First of all, first_key_match isn't used anywhere.

More importantly, the functionality is poorly designed. You can't safely 
mix first_key_match DNs and non-first_key_match DNs, because then you'll 
never know what the object will do.
Whether you get a single value or a list of all shouldn't depend on the 
object, but should be decided by the call -- I recommend `dn['cn']` 
versus `dn.get_all('cn')`.
Furthermore, first_key_match complicates the DN code unnecessarily.

This can be done a subsequent patch; don't let it hold the DN work back.

-- 
Petr³