[Freeipa-devel] [PATCH] 55 Parse comma-separated lists of values in all parameter types

[Jan Cholasta]

Dne 4.11.2011 22:25, Rob Crittenden napsal(a):
> Jan Cholasta wrote:
>> Dne 24.10.2011 17:42, Rob Crittenden napsal(a):
>>> Jan Cholasta wrote:
>>>> Dne 20.10.2011 13:20, Jan Cholasta napsal(a):
>>>>> Parse comma-separated lists of values in all parameter types. This can
>>>>> enabled for a specific parameter by setting the "csvlist" option to
>>>>> True.
>>>>>
>>>>> Remove List parameter type and replace all occurences with Str with
>>>>> csvlist enabled.
>>>>>
>>>>> https://fedorahosted.org/freeipa/ticket/2007
>>>>>
>>>>> This change will be useful for
>>>>> https://fedorahosted.org/freeipa/ticket/1487 and
>>>>> https://fedorahosted.org/freeipa/ticket/1847
>>>>>
>>>>> Unit tests show no regressions.
>>>>>
>>>>> Honza
>>>>>
>>>>
>>>> Self-NACK - I have noticed that the batch command no longer works.
>>>>
>>>> Updated patch attached.
>>>>
>>>> Honza
>>>
>>> What is the benefit of this over the List parameter type?
>>>
>>> rob
>>
>> Mainly because the List parameter type is just a hack. This is the right
>> thing to do if we want to use comma-separated lists of parameters of any
>> type, with all the validation and other parameter type-specific features.
>>
>> For example, I've added a new parameter type for IP addresses in my
>> patch 46
>> (http://www.redhat.com/archives/freeipa-devel/2011-September/msg00187.html)
>>
>> and use it for A and AAAA DNS records. Without this patch, we can either
>> use List for the record parameters and lose validation in dnsrecord-find
>> (because it is based on crud.Search, which strips all the custom
>> validation rules - like _validate_ipaddr - from the command parameters,
>> which is one of the causes of #1627) or use IPAddress for the record
>> parameters and lose the ability to specify them as comma-separated list
>> of values. With this patch, we can have both comma-separated lists and
>> validation at the same time.
>>
>> Besides, the patch is not as big as it looks like, all the interesting
>> stuff is in ipalib/parameters.py, everything else is just
>> search-and-replace. Also I need it to fix #1487 and #1847 without doing
>> ugly hacks.
>>
>> Honza
>>
>
> I think this would constitute a major version change.

I'm not sure about that, as the patch doesn't break API compatibility - 
a string containing a comma-separated list of values is used for list 
parameters both with and without the patch.

>
> One downside is you can no longer tell in the help with arguments take a
> CSV and which don't.

Why not? A simple look at csvlist value should provide enough information.

>
> I think the CSV-related Parameter options should all begin with csv,
> separator and skipspace.

OK.

>
> The batch command may eventually be made into a command, how will that
> affect the Any type?

Batch currently uses List for the "methods" parameter not because of its 
CSV capability, but because it doesn't do any type conversion and 
validation on the values. This allows it to use values of the form 
"{u'params': [args_list, kwargs_dict], u'method': method_name}". The Any 
parameter type exists so that this can still be done without List - it's 
basically a single-valued version of List (i.e. Any(csvlist=True) is 
equivalent to List()).

IMO if batch is ever made into a command, it would have to be redesigned 
not to use List/Any, so that it can be used from CLI with validation of 
the parameter values. Any can then be easily removed.

>
> It otherwise seems to work in my spot-testing.
>
> rob

Honza

-- 
Jan Cholasta