Merge ~jslarraz/review-tools:move-basecontainer-attributes-to-properties into review-tools:master

[Jorge Sancho Larraz]

move basecontainer attributes to properties

Using properties bring some benefits:

- Related functionality does not need to be called during the initialization but only when it is needed. It simplify unittest avoiding the need to mock related functions under certain circunstances
- Properties may not have a setter. That avoid some error during the development as the property cannot be unexpectedly modified (it needs to be done explicitly via the related _attribute)

[Alex Murray (alexmurray)]

Nice improvement - LGTM! I wonder if we should have a HACKING.md or similar which describes the preferred coding conventions going forward?

review: Approve

diff --git a/reviewtools/containers/base_container.py b/reviewtools/containers/base_container.py
index a9e0f41..173036a 100644
--- a/reviewtools/containers/base_container.py
+++ b/reviewtools/containers/base_container.py
@@ -42,8 +42,9 @@ class BaseContainer:
         "application/x-pie-executable",
     ]
 
-    files = None
-    bin_files = None
+    _unpack_dir = None
+    _files = None
+    _bin_files = None
 
     def __init__(self, fn):
         self.tmp_dir = mkdtemp(prefix=MKDTEMP_PREFIX, dir=MKDTEMP_DIR)
@@ -58,16 +59,42 @@ class BaseContainer:
         else:
             raise ContainerException("Could not find '%s'" % fn)
 
-        self.unpack_dir = ".".join(self.filename.split(".")[:-1])
-        unpack_pkg(fn, dest=self.unpack_dir)
+    @property
+    def unpack_dir(self) -> str:
+        """
+        Absolute path to unpack directory temporary location. If it is not defined,
+        then unpack_pkg() will be called and the path of the newly created directory
+        will be returned.
+        """
+        if self._unpack_dir is None:
+            self._unpack_dir = ".".join(self.filename.split(".")[:-1])
+            unpack_pkg(self.filename, dest=self._unpack_dir)
+        return self._unpack_dir
 
-        # Get a list of all unpacked files
-        if self.files is None:
-            self.files = self.get_files_list()
+    @property
+    def files(self) -> list:
+        """
+        List of files present in the container. List elements will be the absolute
+        path of the file taking unpack_dir as root directory. If it is not defined
+        (i.e. the list has not been populated yet), then get_file_list() will be
+        called and the newly created list will be returned.
+        """
+        if self._files is None:
+            self._files = self.get_files_list()
+        return self._files
 
-        # Setup what is needed to get a list of all unpacked compiled binaries
-        if self.bin_files is None:
-            self.bin_files = self.get_compiled_binaries_list()
+    @property
+    def bin_files(self) -> list:
+        """
+        List of compiled binaries present in the container. List elements will be
+        the absolute path of the file taking unpack_dir as root directory. If it is
+        not defined (i.e. the list has not been populated yet), then
+        get_compiled_binaries_list() will be called and the newly created list will
+        be returned.
+        """
+        if self._bin_files is None:
+            self._bin_files = self.get_compiled_binaries_list()
+        return self._bin_files
 
     def __del__(self):
         """
diff --git a/reviewtools/sr_tests.py b/reviewtools/sr_tests.py
index 54c2a6f..6caae54 100644
--- a/reviewtools/sr_tests.py
+++ b/reviewtools/sr_tests.py
@@ -45,9 +45,9 @@ def _mock_func(self):
 class _BaseContainer:
     def __init__(self, fn):
         self.filename = fn
-        self.unpack_dir = ".".join(self.filename.split(".")[:-1])
-        self.files = []
-        self.bin_files = []
+        self._unpack_dir = ".".join(self.filename.split(".")[:-1])
+        self._files = []
+        self._bin_files = []
         self.tmp_dir = "/faketmpdir"