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

[Paulo Ricardo Paz Vital]

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



> 
> >> +    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

>