Resolve LGTM reported errors and warnings (#248)

* BaseException issue reported by LGTM. Don't use empty except as this catches KeyboardInterrupt and SystemExits as well. To keep same behavior use Exception as this will catch errors as expected. * Fix staticmethods in WebDependency set two functions to static to match usage. Remove function get_internal_path_root as it is never used. Remove unit tests for function. * LGTM remove unreachable code and unused variables Co-authored-by: Bret Barkelew <[email protected]>
tianocore · May 20, 2021 · 0a9eb12 · 0a9eb12
1 parent 882e6ed
commit 0a9eb12
Show file tree

Hide file tree

Showing 13 changed files with 25 additions and 41 deletions.
diff --git a/edk2toolext/base_abstract_invocable.py b/edk2toolext/base_abstract_invocable.py
@@ -93,9 +93,9 @@ def ConfigureLogging(self):
 
         md_log_file = self.GetLoggingLevel("md")
         if(md_log_file is not None):
-            md_file, md_logger = edk2_logging.setup_markdown_logger(log_directory,
-                                                                    self.GetLoggingFileName("md"),
-                                                                    md_log_file)
+            edk2_logging.setup_markdown_logger(log_directory,
+                                               self.GetLoggingFileName("md"),
+                                               md_log_file)
 
         logging.info("Log Started: " + datetime.strftime(datetime.now(), "%A, %B %d, %Y %I:%M%p"))
 

diff --git a/edk2toolext/edk2_git.py b/edk2toolext/edk2_git.py
@@ -68,7 +68,6 @@ def _update_from_git(self):
                 self._logger.error("GIT ERROR for {0}".format(self._path))
                 self._logger.error(e)
                 raise e
-                return False
 
     def _get_submodule_list(self):
         submodule_list = []
@@ -273,6 +272,6 @@ def clone_from(self, url, to_path, branch=None, shallow=False, reference=None, *
             params = ["submodule", "update", "--init", "--recursive"]
             params.append("--reference %s" % reference)
             param_string = " ".join(params)
-            ret = RunCmd(cmd, param_string)
+            RunCmd(cmd, param_string)
 
         return Repo(to_path)
diff --git a/edk2toolext/edk2_invocable.py b/edk2toolext/edk2_invocable.py
@@ -127,7 +127,7 @@ def GetLoggingLevel(self, loggerType):
             level = self.PlatformSettings.GetLoggingLevel(loggerType)
             if level is not None:
                 return level
-        except:
+        except Exception:
             pass
 
         if(loggerType == "con") and not self.Verbose:
@@ -199,9 +199,8 @@ def ParseCommandLineOptions(self):
                 imported_classes = ", ".join(classNameList)  # Join the classes together
                 print(f"The module you imported contains {imported_classes}")
             except Exception:
-                # Otherwise, oh well we'll just ignore this.
                 raise
-                pass
+
             settingsParserObj.print_help()
             sys.exit(1)
 

diff --git a/edk2toolext/environment/conf_mgmt.py b/edk2toolext/environment/conf_mgmt.py
@@ -87,7 +87,7 @@ def _get_version(self, conf_file: str) -> str:
                     try:
                         version = str(float(line.split("=")[1].split()[0].strip()))
                         break
-                    except:
+                    except Exception:
                         pass
         return version
 
@@ -108,7 +108,7 @@ def _is_older_version(self, conf_file: str, template_file: str) -> bool:
             logging.debug("Conf version: %s", str(conf))
             template = float(self._get_version(template_file))
             logging.debug("Template Version: %s", str(template))
-        except:
+        except Exception:
             logging.error("Failed to get version from file")
         finally:
             return (conf < template)

diff --git a/edk2toolext/environment/environment_descriptor_files.py b/edk2toolext/environment/environment_descriptor_files.py
@@ -38,7 +38,7 @@ def __init__(self, file_path):
         with open(file_path, 'r') as file:
             try:
                 self.descriptor_contents = yaml.safe_load(file)
-            except:
+            except Exception:
                 pass  # We'll pick up this error when looking at the data.
 
         #

diff --git a/edk2toolext/environment/extdeptypes/web_dependency.py b/edk2toolext/environment/extdeptypes/web_dependency.py
@@ -47,12 +47,14 @@ def __str__(self):
         """ return a string representation of this """
         return f"WebDependecy: {self.source}@{self.version}"
 
+    @staticmethod
     def linuxize_path(path):
         '''
         path: path that uses os.sep, to be replaced with / for compatibility with zipfile
         '''
         return "/".join(path.split("\\"))
 
+    @staticmethod
     def unpack(compressed_file_path, destination, internal_path, compression_type):
         '''
         compressed_file_path: name of compressed file to unpack.
@@ -87,11 +89,6 @@ def unpack(compressed_file_path, destination, internal_path, compression_type):
             _ref.extract(member=file, path=destination)
         _ref.close()
 
-    def get_internal_path_root(outer_dir, internal_path):
-        temp_path_root = internal_path.split(os.sep)[0] if os.sep in internal_path else internal_path
-        unzip_root = os.path.join(outer_dir, temp_path_root)
-        return unzip_root
-
     def fetch(self):
         url = self.source
         temp_folder = tempfile.mkdtemp()

diff --git a/edk2toolext/environment/external_dependency.py b/edk2toolext/environment/external_dependency.py
@@ -117,7 +117,6 @@ def clean(self):
     def fetch(self):
         # The base class does not implement a fetch.
         logging.critical("Fetch() CALLED ON BASE EXTDEP CLASS!")
-        pass
 
     def verify(self):
         result = True

diff --git a/edk2toolext/environment/repo_resolver.py b/edk2toolext/environment/repo_resolver.py
@@ -235,7 +235,7 @@ def checkout(abs_file_system_path, dep, repo, update_ok=False, ignore_dep_state_
                 # try to fetch it and try to checkout again
                 logger.info("We failed to checkout this branch, we'll try to fetch")
                 repo.fetch(branch=branch)
-                result = repo.checkout(branch=branch)
+                repo.checkout(branch=branch)
             repo.submodule("update", "--init", "--recursive")
         else:
             if repo.active_branch == dep["Branch"]:

diff --git a/edk2toolext/environment/uefi_build.py b/edk2toolext/environment/uefi_build.py
@@ -172,10 +172,20 @@ def Go(self, WorkSpace, PackagesPath, PInHelper, PInManager):
                     logging.critical("Flash Image failed")
                     return ret
 
-        except:
+        except Exception:
             logging.critical("Build Process Exception")
             logging.error(traceback.format_exc())
             return -1
+
+        except SystemExit:
+            logging.critical("Build Process Exit")
+            logging.error(traceback.format_exc())
+            return -1
+
+        except KeyboardInterrupt:
+            logging.critical("Build Cancelled by user")
+            return -2
+
         finally:
             end_time = time.perf_counter()
             elapsed_time_s = int((end_time - start_time))

diff --git a/edk2toolext/environment/version_aggregator.py b/edk2toolext/environment/version_aggregator.py
@@ -32,7 +32,6 @@ def ReportVersion(self, key, value, versionType, path=None):
             old_version = self._Versions[key]
             if old_version["version"] == value and old_version["path"] == path:
                 self._logger.info(f"version_aggregator: {key} re-registered at {path}")
-                pass
             else:
                 error = "version_aggregator: {0} key registered with a different value\n\t" \
                         "Old:{1}@{3}\n\tNew:{2}@{4}\n".format(

diff --git a/edk2toolext/omnicache.py b/edk2toolext/omnicache.py
@@ -160,7 +160,6 @@ def InitOmnicache(path):
 
 def AddEntry(config, name, url, tags=False):
     logging.info("Adding remote ({0} : {1}) to Omnicache".format(name, url))
-    param = "remote add {0} {1}".format(name, url)
 
     if config.Contains(name):
         logging.info("Updating remote ({0} : {1}) in Omnicache".format(name, url))

diff --git a/edk2toolext/tests/test_web_dependency.py b/edk2toolext/tests/test_web_dependency.py
@@ -211,24 +211,6 @@ def test_sha256_lowercase_single_file(self):
         if not os.path.isfile(file_path):
             self.fail("The downloaded file isn't there")
 
-    # Test that get_internal_path_root works the way we expect with a flat directory structure.
-    # test_dir\inner_dir - test_dir\inner_dir should be the root.
-    def test_get_internal_path_root_flat(self):
-        outer_dir = test_dir
-        inner_dir_name = "inner_dir"
-        inner_dir_path = os.path.join(outer_dir, inner_dir_name)
-        self.assertEqual(WebDependency.get_internal_path_root(outer_dir, inner_dir_name), inner_dir_path)
-
-    # Test that get_internal_path_root works the way we expect with a flat directory structure
-    # test_dir\first_dir\second_dir - test_dir\first_dir should be the root
-    def test_get_internal_path_root_with_subfolders(self):
-        outer_dir = test_dir
-        first_level_dir_name = "first_dir"
-        second_level_dir_name = "second_dir"
-        inner_dir_path = os.path.join(outer_dir, first_level_dir_name)
-        inner_second_dir_path = os.path.join(first_level_dir_name, second_level_dir_name)
-        self.assertEqual(WebDependency.get_internal_path_root(outer_dir, inner_second_dir_path), inner_dir_path)
-
     # Test that a single file zipped is able to be processed by unpack.
     def test_unpack_zip_file(self):
         compressed_file_path = os.path.join(test_dir, "bad_ext_dep_zip.zip")

diff --git a/setup.py b/setup.py
@@ -29,7 +29,7 @@ class PostInstallCommand(install):
     def run(self):
         try:
             DownloadNuget()
-        except:
+        except Exception:
             pass
         install.run(self)
 
@@ -39,7 +39,7 @@ class PostDevCommand(develop):
     def run(self):
         try:
             DownloadNuget()
-        except:
+        except Exception:
             pass
         develop.run(self)