Hi Milo,

Thanks for working on this.

У уто, 29. 05 2012. у 07:40 +0000, Milo Casagrande пише:

> as per bug 727776, I'm proposing the following branch for merge.
> 
> I added a new test case for the '--hwpack' argument, checking it
> against a temp directory and raising an InvalidHwpackFile exception. I
> added then a new check in additional_option_checkers to check that the
> argument passed for '--hwpack' is a regular file.
> 
> For the test case I based myself on the already defined test cases. I
> only had to define my Args class in order to pass the valid arguments.

> I ran the tests suite and it runs without errors.
> Thanks.

Cool, it's worth running pep8 on the files you've modified as well:

$ pep8 linaro_image_tools/tests/test_utils.py linaro_image_tools/utils.py
linaro_image_tools/tests/test_utils.py:90:5: E303 too many blank lines (2)
linaro_image_tools/tests/test_utils.py:140:1: W293 blank line contains whitespace
linaro_image_tools/tests/test_utils.py:198:1: E302 expected 2 blank lines, found 1
linaro_image_tools/tests/test_utils.py:321:40: W291 trailing whitespace
linaro_image_tools/utils.py:46:80: E501 line too long (81 characters)
linaro_image_tools/utils.py:107:14: E111 indentation is not a multiple of four
linaro_image_tools/utils.py:221:24: E231 missing whitespace after ','
linaro_image_tools/utils.py:256:17: E261 at least two spaces before inline comment
linaro_image_tools/utils.py:297:1: W391 blank line at end of file

(most/all are probably not your faults, but they'd be nice to clean-up
so we work towards a cleaner looking code :)

> разлике међу датотекама прилог (review-diff.txt)
> === modified file 'linaro_image_tools/tests/test_utils.py'
> --- linaro_image_tools/tests/test_utils.py	2012-04-18 13:26:25 +0000
> +++ linaro_image_tools/tests/test_utils.py	2012-05-29 07:39:22 +0000
> @@ -44,6 +44,7 @@
>      IncompatibleOptions,
>      prep_media_path,
>      additional_option_checks,
> +    InvalidHwpackFile,
>      )

It would be good to sort these alphabetically as well: that makes it
easier to see if an import is already there or not when one tries to add
something.

And if there are many problems like this (and pep8 stuff) that one
notices during development, it's usually better to leave them for after
the review (or review might be harder).

>  sudo_args = " ".join(cmd_runner.SUDO_ARGS)
> @@ -286,6 +287,7 @@
>                                                device="testdevice",
>                                                board="testboard")))
>  
> +

Thanks for fixing this one :)

>  class TestPrepMediaPath(TestCaseWithFixtures):
>  
>      def test_additional_option_checks(self):
> @@ -303,3 +305,21 @@
>                                 device="testdevice",
>                                 board="testboard"))
>          sys.argv.remove("--mmc")
> +
> +
> +class TestHwpackIsFile(TestCaseWithFixtures):
> +
> +    """Testing '--hwpack' option only allows file."""

"only allows regular files." sounds a bit better and is more
descriptive.

Rest of the test looks good.

> === modified file 'linaro_image_tools/utils.py'
> --- linaro_image_tools/utils.py	2012-04-18 13:26:25 +0000
> +++ linaro_image_tools/utils.py	2012-05-29 07:39:22 +0000
> @@ -266,6 +266,10 @@
>      """We can't find a package which provides the given command."""
>  
> 
> +class InvalidHwpackFile(Exception):
> +    """The hwpack parameter is not a regular file."""
> +
> +
>  class IncompatibleOptions(Exception):
>      def __init__(self, value):
>          self.value = value
> @@ -285,3 +289,9 @@
>          if re.search(r"^/", args.device):
>              raise IncompatibleOptions("--directory option incompatable with "

Now that I am looking at this code, it'd be nice to fix the above typo:

  s/incompatable/incompatible/


>                                        "a full path in --image-file")
> + 
> +    for hwpack in args.hwpacks:
> +        if not os.path.isfile(hwpack):
> +            raise InvalidHwpackFile(
> +                "--hwpack argument (%s) is not a regular file" % hwpack)
> +     

The code here actually brings up one point: it might be useful to add a
test to ensure multiple hwpacks are handled properly as well.  I.e. we
can have a hwpack pointing to a real file, and only the second one
pointing to a directory or non-existing file.

This would ensure that if someone touches this code and decides to just
check for the first argument, they get told about how they've broke it
by the test failing :)

  review needs-fixing