Merge lp:~milo/linaro-image-tools/bug1073973 into lp:linaro-image-tools/11.11

[Milo Casagrande]

The proposed branch fixes the duplicate error shown: the bootloader attribute setter and getter have been reworked. Instead of performing the lookup of the bootloader during the setter operation, now it is done during the getter one, setting the internal bootloader
variable if it is none.

Some other minor refactorings have been performed.

[lp:~milo/linaro-image-tools/bug1073973]

602. By Milo Casagrande on 2013-02-05

Merged from trunk.

[Stevan Radaković (stevanr)]

Hey Milo, this looks good,

The diff part with method removal of set_board confused me a bit, didn't see anything about it in the description

One question regarding

86 +
87 + bootloader = property(_get_bootloader, _set_bootloader)

Shouldn't we put this property definition on the top of the class? it really looks strange being in the middle of method definition...

[Milo Casagrande (milo)]

Thanks Stevan for looking into it!

On Tue, Feb 12, 2013 at 10:04 AM, Stevan Radaković
<email address hidden> wrote:
> Hey Milo, this looks good,
>
> The diff part with method removal of set_board confused me a bit, didn't see anything about it in the description

Yeah... for me that was part of the minor refactorings, could have
marked that more promptly though. :-/
I removed it because it was not strictly necessary, since there is not
real data hiding and the property was not "internal" to the class, it
can be easily accessed without the function.

> One question regarding
>
> 86 +
> 87 + bootloader = property(_get_bootloader, _set_bootloader)
>
> Shouldn't we put this property definition on the top of the class? it really looks strange being in the middle of method definition...

I'm not following here... the property definition is after the
__init__ and after the two property methods (_get and _set). I cannot
move it before those or at the top of the class, or when accessing it
Python will complain that the methods are not defined.
We would have to move everything, the property and the methods, before
the __init__, but from a logical point of view I would say to keep
them after the __init__.

Ciao.

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

[Stevan Radaković (stevanr)]

> Thanks Stevan for looking into it!
>
> On Tue, Feb 12, 2013 at 10:04 AM, Stevan Radaković
> <email address hidden> wrote:
> > Hey Milo, this looks good,
> >
> > The diff part with method removal of set_board confused me a bit, didn't see
> anything about it in the description
>
> Yeah... for me that was part of the minor refactorings, could have
> marked that more promptly though. :-/
> I removed it because it was not strictly necessary, since there is not
> real data hiding and the property was not "internal" to the class, it
> can be easily accessed without the function.

Cool, clear..

>
> > One question regarding
> >
> > 86 +
> > 87 + bootloader = property(_get_bootloader, _set_bootloader)
> >
> > Shouldn't we put this property definition on the top of the class? it really
> looks strange being in the middle of method definition...
>
> I'm not following here... the property definition is after the
> __init__ and after the two property methods (_get and _set). I cannot
> move it before those or at the top of the class, or when accessing it
> Python will complain that the methods are not defined.
> We would have to move everything, the property and the methods, before
> the __init__, but from a logical point of view I would say to keep
> them after the __init__.
>

Oh ok I wasn't aware of this, thought you can define property first and then methods later.
Thanks for explaining.

Anyway, long overdue, approve +1 :)

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

review: Approve

=== modified file 'linaro_image_tools/hwpack/builder.py'
--- linaro_image_tools/hwpack/builder.py	2012-10-18 13:09:21 +0000
+++ linaro_image_tools/hwpack/builder.py	2013-02-05 10:08:38 +0000
@@ -185,16 +185,16 @@
         """Call function for each board + bootloader combination in metadata"""
         if self.config.bootloaders is not None:
             for bootloader in self.config.bootloaders:
-                self.config.set_board(None)
-                self.config.set_bootloader(bootloader)
+                self.config.board = None
+                self.config.bootloader = bootloader
                 function()
 
         if self.config.boards is not None:
             for board in self.config.boards:
                 if self.config.bootloaders is not None:
                     for bootloader in self.config.bootloaders:
-                        self.config.set_board(board)
-                        self.config.set_bootloader(bootloader)
+                        self.config.board = board
+                        self.config.bootloader = bootloader
                         function()
 
     def extract_files(self):
 
=== modified file 'linaro_image_tools/hwpack/config.py'
--- linaro_image_tools/hwpack/config.py	2013-01-03 10:40:12 +0000
+++ linaro_image_tools/hwpack/config.py	2013-02-05 10:08:38 +0000
@@ -160,7 +160,8 @@
         # self.allow_unset_bootloader allows for both modes of operation.
         self.logger = logging.getLogger('linaro_image_tools')
         self.allow_unset_bootloader = allow_unset_bootloader
-        self.bootloader = None
+        self.board = board
+        self._bootloader = bootloader
 
         obfuscated_e = None
         obfuscated_yaml_e = ""
@@ -183,10 +184,6 @@
             else:
                 # If YAML parsed OK, we don't have an error.
                 obfuscated_e = None
-                if board:
-                    self.set_board(board)
-                if bootloader:
-                    self.set_bootloader(bootloader)
 
         if obfuscated_e:
             # If INI parsing from ConfigParser or YAML parsing failed,
@@ -198,12 +195,12 @@
                    obfuscated_yaml_e)
             raise ConfigParser.Error(msg)
 
-    def set_bootloader(self, bootloader):
-        """Set bootloader used to look up configuration in bootloader section.
+    def _get_bootloader(self):
+        """Returns the bootloader associated with this config.
 
         If bootloader is None / empty and there is only one bootloader
-        available, use that.
-        """
+        available, use that."""
+        bootloader = self._bootloader
         if not bootloader:
             # Auto-detect bootloader. If there is a single bootloader specified
             # then use it, else, error.
@@ -227,12 +224,17 @@
                                             'found. Will try to use \'%s\'. '
                                             'instead.' % (DEFAULT_BOOTLOADER,
                                                           bootloader))
-
-        self.bootloader = bootloader
-
-    def set_board(self, board):
-        """Set board used to look up per-board configuration"""
-        self.board = board
+            # bootloader is None: since we are here, set it so we do not
+            # have to go through all the config retrieval again.
+            self._bootloader = bootloader
+        return bootloader
+
+    def _set_bootloader(self, value):
+        """Set bootloader used to look up configuration in bootloader section.
+        """
+        self._bootloader = value
+
+    bootloader = property(_get_bootloader, _set_bootloader)
 
     def get_bootloader_list(self):
         if isinstance(self.bootloaders, dict):
@@ -272,7 +274,7 @@
             # Check config for all bootloaders if one isn't specified.
             if not self.bootloader and self._is_v3:
                 for bootloader in self.get_bootloader_list():
-                    self.set_bootloader(bootloader)
+                    self.bootloader = bootloader
                     self.validate_bootloader_fields()
             else:
                 self.validate_bootloader_fields()
 
=== modified file 'linaro_image_tools/hwpack/handler.py'
--- linaro_image_tools/hwpack/handler.py	2012-12-07 09:37:42 +0000
+++ linaro_image_tools/hwpack/handler.py	2013-02-05 10:08:38 +0000
@@ -105,8 +105,8 @@
                 # Probably V2 hardware pack without [hwpack] on the first line
                 lines = ["[hwpack]\n"] + lines
             self.config = Config(StringIO("".join(lines)))
-            self.config.set_board(self.board)
-            self.config.set_bootloader(self.bootloader)
+            self.config.board = self.board
+            self.config.bootloader = self.bootloader
         return self.config
 
     def get_field(self, field, return_keys=False):
 
=== modified file 'linaro_image_tools/hwpack/tests/test_config_v3.py'
--- linaro_image_tools/hwpack/tests/test_config_v3.py	2013-01-03 10:40:12 +0000
+++ linaro_image_tools/hwpack/tests/test_config_v3.py	2013-02-05 10:08:38 +0000
@@ -245,7 +245,7 @@
                                  "bootloaders:\n"
                                  " u_boot:\n"
                                  "  spl_package: ~~\n")
-        config.set_board("panda")
+        config.board = "panda"
         self.assertValidationError(
             "Invalid value in spl_package in the metadata: ~~",
             config._validate_spl_package)
@@ -257,7 +257,7 @@
                                  "  bootloaders:\n"
                                  "   u_boot:\n"
                                  "    spl_file: ~~\n")
-        config.set_board("panda")
+        config.board = "panda"
         self.assertValidationError("Invalid path: ~~",
                                    config._validate_spl_file)
 
@@ -297,8 +297,8 @@
             "   u_boot:\n"
             "    in_boot_part: Yes\n")
 
-        config.set_bootloader("u_boot")
-        config.set_board("panda")
+        config.bootloader = "u_boot"
+        config.board = "panda"
 
         config._validate_bootloader_file_in_boot_part()
         self.assertEqual(config.bootloader_file_in_boot_part, "yes")
@@ -315,8 +315,8 @@
             "   u_boot:\n"
             "    in_boot_part: Yes\n")
 
-        config.set_bootloader("u_boot")
-        config.set_board("panda")
+        config.bootloader = "u_boot"
+        config.board = "panda"
 
         config._validate_bootloader_file_in_boot_part()
         self.assertEqual(config.bootloader_file_in_boot_part, "yes")
@@ -330,11 +330,11 @@
             " anotherboot:\n"
             "  in_boot_part: Yes\n")
 
-        config.set_bootloader("u_boot")
+        config.bootloader = "u_boot"
         config._validate_bootloader_file_in_boot_part()
         self.assertEqual(config.bootloader_file_in_boot_part, "no")
 
-        config.set_bootloader("anotherboot")
+        config.bootloader = "anotherboot"
         config._validate_bootloader_file_in_boot_part()
         self.assertEqual(config.bootloader_file_in_boot_part, "yes")
 
 
=== modified file 'linaro_image_tools/hwpack/tests/test_script.py'
--- linaro_image_tools/hwpack/tests/test_script.py	2012-08-29 15:01:41 +0000
+++ linaro_image_tools/hwpack/tests/test_script.py	2013-02-05 10:08:38 +0000
@@ -168,7 +168,7 @@
         # We now need a real config object to test against the configuration
         # in the hardware pack we have created.
         config = Config(StringIO(config_v3))
-        config.set_bootloader("u_boot")
+        config.bootloader = "u_boot"
         metadata = Metadata.from_config(config, "1.0", "armel")
         self.assertThat(
             "hwpack_ahwpack_1.0_armel.tar.gz",