[Kimchi-devel] [PATCH 1/2] Add function to convert data sizes

[Aline Manera]

On 03/03/2015 11:00, Paulo Ricardo Paz Vital wrote:
> On Tue, 2015-03-03 at 10:21 -0300, Crístian Viana wrote:
>> On 03-03-2015 05:16, Paulo Ricardo Paz Vital wrote:
>>> s/src_unit/src_unit=None
>>> As explained below, both second and third arguments can be None and then
>>> automatically converted to use DEFAULT_UNIT.
>>>
>>> I tried "convert_data_size(4)" and got the error:
>>> TypeError: convert_data_size() takes at least 2 arguments (1 given)
>> I didn't mean the first parameter to be optional. What would you expect
>> when calling "convert_data_size(4)"? Assuming both units as bytes, the
>> function would always return the same value which was passed to it. Both
>> parameters *can* be None but that doesn't mean we should create a
>> default situation where it happens, because that wouldn't be useful at all.
>>
> OK, so I think you should do some modifications on code to be coherent
> with what you intend to do.
>
> First, you should re-write the documentation part where you explain the
> parameters, by removing the line "if 'src_unit' is empty, the unit
> 'B' (byte) will be used.", because it will never happen.
>
> Second, it's not necessary the following block, again, because it will
> never happen. Removing this block, you will guarantee that 'src_unit'
> and 'dst_unit' will be always different and you will not create a
> default situatio.
>
> +    if not src_unit:
> +        src_unit = DEFAULT_UNIT
>

I don't think we should use None as a default parameter and then covert 
it to a real parameter.
It should use the real parameter in function.

def convert_data_size(value, src_unit, dst_unit='B'):

And make the first and second parameters required. So we always need to 
pass the value and its unit to do the conversion.
And the default destination unit is 'B'.

>
>>>> +    SI_PREFIXES = ['k', 'M', 'G', 'T', 'P', 'E', 'Z', 'Y']
>>> I guess you can add a new line here, as you did between
>>> SUFFIXES_WITH_MULT and DEFAULT_SUFFIX.
>>>
>>>> +    # The IEC prefixes are the equivalent SI prefixes + 'i'
>>>> +    # but, exceptionally, 'k' becomes 'Ki' instead of 'ki'.
>>>> +    IEC_PREFIXES = map(lambda p: 'Ki' if p == 'k' else p + 'i', SI_PREFIXES)
>>> New line here.
>>>
>>>> +    PREFIXES_BY_BASE = {1000: SI_PREFIXES,
>>>> +                        1024: IEC_PREFIXES}
>>>> +
>>>> +    SUFFIXES_WITH_MULT = {'b': 1,
>>>> +                          'B': 8}
>>>> +    DEFAULT_SUFFIX = 'B'
>>>> +
>>>> +    DEFAULT_UNIT = 'B'
>>>> +
>>>> +
>> Well, I tried to group the variable declarations by subject:
>> prefix-related variables, <blank space>, suffix-related variables,
>> <blank space>, unit-related variable.
> Got it!
> However, I'm "old school" and learned that if you add a comment, you, as
> a best practice, should add a blank line between the previous line and
> the comment line :-P
>
>
> _______________________________________________
> Kimchi-devel mailing list
> Kimchi-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/kimchi-devel