Linaro Image Tools

Code review comment for lp:~milo/linaro-image-tools/bug1004199

bug1004199
Merge into trunk

Revision history for this message

Milo Casagrande (milo) wrote on 2012-06-06:

Hello Danilo,

On Wed, Jun 6, 2012 at 9:55 AM, Данило Шеган <email address hidden> wrote:
>
> With these changes, the name of the method might not make that much
> sense anymore. It should be
>
> round_to_megs_ensure_larger_than_one_meg_and_convert_to_bytes :)
>
> Perhaps use something that describes the nature of the method is now
> more appropriate (i.e. "get_partition_size_in_bytes" or something
> similar).

I will use your suggestion for the method name. :-)

>> I added also a new method, _check_min_size, to ensure that we have at least 1MiB image size (this was the bit discussed on IRC).
>
> It would be good to add a few minimal tests for it as well :)

Added two new tests: one for a smaller value, one for a bigger one.
I added also a comment on _check_min_size, in order to avoid testing
passing size as a string: in the function "size" is a number. I used
"@param", is it the blessed way or is it better to use ":param"?

> I'd rather if we express this as two separate values, even if one is
> defined through the other. Eg.
>
> ROUND_IMAGE_TO = 2 ** 20
> MIN_IMAGE_SIZE = ROUND_IMAGE_TO

Fixed.

>> @@ -481,11 +484,12 @@
>> def convert_size_to_bytes(size):
>> """Convert a size string in Kbytes, Mbytes or Gbytes to bytes."""
>
> It'd be nice to extend this to document the new features (min size,
> rounding).

Added a small description of what is happening, another note on this
follows on the comment below.

>>...
>> + # Guarantee that is a multiple of 1MiB, defined in ROUND_IMAGE_TO
>> + real_size = _check_min_size(floor(real_size / ROUND_IMAGE_TO) *
>> + ROUND_IMAGE_TO)
>
> Better way to do the rounding is always to add half of ROUND_IMAGE_TO
> and then simply round() it. This will ensure that you've got enough
> space at all times.
> Eg.
>
> round((real_size + ROUND_IMAGE_TO/2)/ROUND_IMAGE_TO) * ROUND_IMAGE_TO
>
> will give you an image size which will be able to fit the requested
> number of KBs or MBs (eg. asking for '1024.4K' will give you 2MB which
> can really fit the requested number of kilobytes; it'd be even worse if
> you ask for 1500K and get 1MB).

Hmm... OK, got your point to have always enough space, although in
this case if we ask for '1M' we receive back a size of 2MiB, and
always a little bit more for all the other values. I hope it is not an
issue with space on users side if they want a better precision.

Changing this introduced a regression on another test, specifically
'test_setup_partitions_for_image_file' that tests 'setup_partitions':
image size in bytes here is used with 'dd'.

I changed the behavior of the converting functions, saying also that
the conversion in bytes is "deliberaltey inaccurate" (in the sense
that we add a small delta).
Fixed now, and also the tests and their values. :-)

>> +def _check_min_size(size):
>> + """Check that the image size is at least 1MiB (expressed in bytes)."""
>
> Hard-coding a value here which is not really hard-coded doesn't make
> much sense. Why not make it 'is at least MIN_IMAGE_SIZE bytes.'
> instead?

Fixed.

>> === modified file 'linaro_image_tools/media_create/tests/test_media_create.py'
>
> These are not of your doing, but id' be worth fixing them (and I did
> notice other fixes you did — thanks! :)
>
> $ pep8 linaro_image_tools/media_create/tests/test_media_create.py
> linaro_image_tools/media_create/tests/test_media_create.py:306:9: E301
> expected 1 blank line, found 0
> linaro_image_tools/media_create/tests/test_media_create.py:484:80: E501
> line too long (80 characters)
> linaro_image_tools/media_create/tests/test_media_create.py:1096:1: E302
> expected 2 blank lines, found 1
> linaro_image_tools/media_create/tests/test_media_create.py:2350:42: E225
> missing whitespace around operator

Almost all should be fixed now: there is still one complaining about
line too long, but it is the name of a method, so, do not know how to
deal exactly.

--
Milo Casagrande
Infrastructure Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

Hello Danilo,

On Wed, Jun 6, 2012 at 9:55 AM, Данило Шеган <danilo@canonical.com> wrote:
>
> With these changes, the name of the method might not make that much
> sense anymore.  It should be
>
>  round_to_megs_ensure_larger_than_one_meg_and_convert_to_bytes :)
>
> Perhaps use something that describes the nature of the method is now
> more appropriate (i.e. "get_partition_size_in_bytes" or something
> similar).

I will use your suggestion for the method name. :-)

>> I added also a new method, _check_min_size, to ensure that we have at least 1MiB image size (this was the bit discussed on IRC).
>
> It would be good to add a few minimal tests for it as well :)

> I'd rather if we express this as two separate values, even if one is
> defined through the other. Eg.
>
> ROUND_IMAGE_TO = 2 ** 20
> MIN_IMAGE_SIZE = ROUND_IMAGE_TO

Fixed.

>> @@ -481,11 +484,12 @@
>>  def convert_size_to_bytes(size):
>>      """Convert a size string in Kbytes, Mbytes or Gbytes to bytes."""
>
> It'd be nice to extend this to document the new features (min size,
> rounding).

Added a small description of what is happening, another note on this
follows on the comment below.

>>...
>> +    # Guarantee that is a multiple of 1MiB, defined in ROUND_IMAGE_TO
>> +    real_size = _check_min_size(floor(real_size / ROUND_IMAGE_TO) *
>> +                                ROUND_IMAGE_TO)
>
> Better way to do the rounding is always to add half of ROUND_IMAGE_TO
> and then simply round() it.  This will ensure that you've got enough
> space at all times.
> Eg.
>
>  round((real_size + ROUND_IMAGE_TO/2)/ROUND_IMAGE_TO) * ROUND_IMAGE_TO
>
> will give you an image size which will be able to fit the requested
> number of KBs or MBs (eg. asking for '1024.4K' will give you 2MB which
> can really fit the requested number of kilobytes; it'd be even worse if
> you ask for 1500K and get 1MB).

Changing this introduced a regression on another test, specifically
'test_setup_partitions_for_image_file' that tests 'setup_partitions':
image size in bytes here is used with 'dd'.

>> +def _check_min_size(size):
>> +    """Check that the image size is at least 1MiB (expressed in bytes)."""
>
> Hard-coding a value here which is not really hard-coded doesn't make
> much sense.  Why not make it 'is at least MIN_IMAGE_SIZE bytes.'
> instead?

Fixed.

Almost all should be fixed now: there is still one complaining about
line too long, but it is the name of a method, so, do not know how to
deal exactly.

-- 
Milo Casagrande
Infrastructure Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

« Back to merge proposal