chore: add ruff to QA checks

`ruff` is the hot new linter for Python. It is written in Rust and is
extremely fast. It also has tremendous support and constant updates. It
replaces some of the existing (`pyupgrade`) and planned (e.g., `bandit`,
`flake8`, `isort`, and `pylint`) QA tools that are outlined in #14. More
new rules are added all the time. The initial configuration enables
`ALL` rules by default and then selectively ignores specific rules based
on conflicts, preference, and existing issues in the backlog.

Using `ALL` has the risk that new rules may be enabled when `ruff` is
updated but updates are automated, with a PR that includes enforced QA
checks, to weekly dependency and pre-commit hook bumps. Therefore, the
real risk is that a developer on this project may have a local version
`ruff` installed that is newer than the one used for the `pre-commit`
hook. That should limit the exposure to no more than a week of "early"
rules and may still result in improved code quality.

The list of actions taken in this PR include:

* Add a custom `ruff` configuration in `pyproject.toml`
  * All rules are enabled by default
  * Specific rules and rule sets are disabled
* Remove the `pyupgrade` pre-commit hook
  * Those checks are covered by `ruff`
* Add the `ruff` pre-commit hook
  * This will enforce the rules in the QA check
* Enable GitHub PR annotations for any findings
* Add `noqa` comments, with reasons, where appropriate
* Sort imports based on the `isort` configuration
* Adhere to PEP-257 docstrings everywhere
* Update the `CONTRIBUTING` guide to discuss QA expectations
* Make updates to resolve all outstanding findings
* Add a `black` badge to the `README` to show the formatting style used

.github/workflows/test.yml

@@ -51,6 +51,8 @@ jobs:
           #   * The current GitHub integration expects to *only* be run in a PR context
           #   * The `phylum-ci` action will already be run for pull request triggers
           SKIP: phylum-ci
+          # Add annotations to the PR for any findings
+          RUFF_FORMAT: github
         run: poetry run tox run -e qa
 
   test-matrix:

.pre-commit-config.yaml

@@ -21,11 +21,10 @@ repos:
     hooks:
       - id: black
 
-  - repo: https://github.com/asottile/pyupgrade
-    rev: 7c551d39dfa69851607d85392ffa296568e92436  # frozen: v3.3.2
+  - repo: https://github.com/charliermarsh/ruff-pre-commit
+    rev: aad2f97dee13b682f514df846844b374cdd06e7c  # frozen: v0.0.264
     hooks:
-      - id: pyupgrade
-        args: [--py38-plus]
+      - id: ruff
 
   - repo: https://github.com/dosisod/refurb
     rev: 7fb404137a6dbb8c7b346ffd904db5c17b1c24ed  # frozen: v1.16.0

CONTRIBUTING.md

@@ -213,6 +213,12 @@ The pull request should work for Python 3.8, 3.9, 3.10, and 3.11.
 Check <https://github.com/phylum-dev/phylum-ci/actions> and make sure that the tests
 pass for all supported Python versions.
 
+To ensure quality assurance (QA), a series of checks are performed in a `qa` test.
+This test essentially runs the `pre-commit` hooks to ensure proper formatting and linting over the repo.
+Sometimes it is necessary to bypass these checks (e.g., via `# noqa` directives). These exceptions should
+be rare and include a reason in the comment for the exclusion. Create a new issue and reference that, when
+more detail is needed or the exclusion is meant to be temporary.
+
 The [Semantic Pull Requests](https://github.com/apps/semantic-pull-requests) GitHub app is in use for this repository
 as a means to ensure each PR that gets merged back to the `main` branch adheres to the
 [conventional commit](https://www.conventionalcommits.org) strategy. This means that either the PR title (when

README.md

@@ -8,6 +8,7 @@
 [![GitHub Workflow Status (branch)][workflow_shield]][workflow_test]
 [![Contributor Covenant](https://img.shields.io/badge/Contributor%20Covenant-2.1-4baaaa.svg)][CoC]
 [![pre-commit](https://img.shields.io/badge/pre--commit-enabled-brightgreen?logo=pre-commit)][pre-commit]
+[![Code style: black](https://img.shields.io/badge/code%20style-black-000000.svg)][black]
 [![Downloads](https://pepy.tech/badge/phylum/month)][downloads]
 [![Discord](https://img.shields.io/discord/1070071012353376387?logo=discord)][discord_invite]
 
@@ -19,6 +20,7 @@ Utilities for integrating Phylum into CI pipelines (and beyond)
 [workflow_test]: https://github.com/phylum-dev/phylum-ci/actions/workflows/test.yml
 [CoC]: https://github.com/phylum-dev/phylum-ci/blob/main/CODE_OF_CONDUCT.md
 [pre-commit]: https://github.com/pre-commit/pre-commit
+[black]: https://github.com/psf/black
 [downloads]: https://pepy.tech/project/phylum
 [discord_invite]: https://discord.gg/Fe6pr5eW6p
 

pyproject.toml

@@ -90,6 +90,53 @@ paths = ["src", "tests"]
 python_version = "3.8"
 format = "github"
 
+[tool.ruff]
+# Reference: https://beta.ruff.rs/docs/settings
+line-length = 120
+target-version = "py38"
+force-exclude = true
+src = ["src", "tests"]
+select = [
+    # Using `ALL` has the risk that new rules may be enabled when `ruff` is updated but updates are
+    # automated, with a PR that includes enforced QA checks, to weekly dependency and pre-commit hook bumps.
+    "ALL"
+]
+ignore = [
+    # Reference: https://beta.ruff.rs/docs/rules/
+    #
+    # `one-blank-line-before-class` (D203) and `no-blank-line-before-class` (D211) are incompatible. Prefer D211.
+    "D203", # one-blank-line-before-class
+    # `multi-line-summary-first-line` (D212) and `multi-line-summary-second-line` (D213) are incompatible. Prefer D212.
+    "D213", # multi-line-summary-second-line
+    # Cached instance methods are okay in this project b/c instances are short lived and won't lead to memory leaks.
+    "B019", # cached-instance-method
+    # Assigning to a variable before a return statement is more readable and useful for debugging
+    "RET504",   # unnecessary-assign
+    # These ignores will be removed during https://github.com/phylum-dev/phylum-ci/issues/238
+    "ANN",  # flake8-annotations
+    "TCH",  # flake8-type-checking
+    # These ignores will be removed during https://github.com/phylum-dev/phylum-ci/issues/47
+    "EM",   # flake8-errmsg
+    "TRY",  # tryceratops
+    # This ignore will be removed during https://github.com/phylum-dev/phylum-ci/issues/23
+    "T20",  # flake8-print
+]
+
+[tool.ruff.per-file-ignores]
+"test_*.py" = [
+    # It is expected to use `assert` statements in `pytest` test code
+    "S101", # assert
+    # `subprocess` input is controlled in test code
+    "S603", # subprocess-without-shell-equals-true
+]
+
+[tool.ruff.isort]
+force-sort-within-sections = true
+
+[tool.ruff.pydocstyle]
+# Use PEP-257 style docstrings
+convention = "pep257"
+
 [tool.semantic_release]
 # Reference: https://python-semantic-release.readthedocs.io/en/latest/configuration.html
 branch = "main"

src/phylum/ci/ci_azure.py

@@ -15,15 +15,15 @@
   * https://learn.microsoft.com/rest/api/azure/devops/git/pull-request-threads/list
   * https://learn.microsoft.com/rest/api/azure/devops/git/pull-request-threads/create
 """
+from argparse import Namespace
 import base64
+from functools import cached_property, lru_cache
 import os
 import re
 import shlex
 import subprocess
+from typing import Optional, Tuple
 import urllib.parse
-from argparse import Namespace
-from functools import cached_property, lru_cache
-from typing import Optional
 
 import requests
 
@@ -84,7 +84,7 @@ def is_in_pr() -> bool:
 class CIAzure(CIBase):
     """Provide methods for an Azure Pipelines CI environment."""
 
-    def __init__(self, args: Namespace) -> None:
+    def __init__(self, args: Namespace) -> None:  # noqa: D107 ; the base __init__ docstring is better here
         super().__init__(args)
         self.ci_platform_name = "Azure Pipelines"
         if is_in_pr():
@@ -164,17 +164,7 @@ def common_ancestor_commit(self) -> Optional[str]:
         remote = git_remote()
 
         if is_in_pr():
-            # There is no single predefined variable available to provide the PR base SHA.
-            # Instead, it can be determined with a `git merge-base` command, like is done for the CINone implementation.
-            # Reference: https://learn.microsoft.com/azure/devops/pipelines/build/variables
-            src_branch = os.getenv("SYSTEM_PULLREQUEST_SOURCEBRANCH", "")
-            tgt_branch = os.getenv("SYSTEM_PULLREQUEST_TARGETBRANCH", "")
-            if not src_branch:
-                raise SystemExit(" [!] The SYSTEM_PULLREQUEST_SOURCEBRANCH environment variable must exist and be set")
-            if not tgt_branch:
-                raise SystemExit(" [!] The SYSTEM_PULLREQUEST_TARGETBRANCH environment variable must exist and be set")
-            print(f" [+] SYSTEM_PULLREQUEST_SOURCEBRANCH: {src_branch}")
-            print(f" [+] SYSTEM_PULLREQUEST_TARGETBRANCH: {tgt_branch}")
+            src_branch, tgt_branch = get_pr_branches()
         else:
             # Assume the working context is within a CI triggered build environment when not in a PR.
 
@@ -221,7 +211,7 @@ def common_ancestor_commit(self) -> Optional[str]:
         cmd = ["git", "merge-base", src_branch, tgt_branch]
         print(f" [*] Finding common ancestor commit with command: {shlex.join(cmd)}")
         try:
-            common_commit = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout.strip()
+            common_commit = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout.strip()  # noqa: S603
         except subprocess.CalledProcessError as err:
             ref_url = "https://learn.microsoft.com/azure/devops/pipelines/yaml-schema/steps-checkout#shallow-fetch"
             print(f" [!] The common ancestor commit could not be found: {err}")
@@ -282,6 +272,22 @@ def post_output(self) -> None:
             post_github_comment(comments_url, self.github_token, self.analysis_report)
 
 
+def get_pr_branches() -> Tuple[str, str]:
+    """Get the source and destination branches when in a PR context and return them as a tuple."""
+    # There is no single predefined variable available to provide the PR base SHA.
+    # Instead, it can be determined with a `git merge-base` command, like is done for the CINone implementation.
+    # Reference: https://learn.microsoft.com/azure/devops/pipelines/build/variables
+    src_branch = os.getenv("SYSTEM_PULLREQUEST_SOURCEBRANCH", "")
+    tgt_branch = os.getenv("SYSTEM_PULLREQUEST_TARGETBRANCH", "")
+    if not src_branch:
+        raise SystemExit(" [!] The SYSTEM_PULLREQUEST_SOURCEBRANCH environment variable must exist and be set")
+    if not tgt_branch:
+        raise SystemExit(" [!] The SYSTEM_PULLREQUEST_TARGETBRANCH environment variable must exist and be set")
+    print(f" [+] SYSTEM_PULLREQUEST_SOURCEBRANCH: {src_branch}")
+    print(f" [+] SYSTEM_PULLREQUEST_TARGETBRANCH: {tgt_branch}")
+    return src_branch, tgt_branch
+
+
 def post_azure_comment(azure_token: str, comment: str) -> None:
     """Post a comment on an Azure Repos Pull Request (PR).
 
@@ -366,7 +372,7 @@ def post_azure_comment(azure_token: str, comment: str) -> None:
                 "parentCommentId": 0,
                 "content": comment,
                 "commentType": "text",
-            }
+            },
         ],
         "status": "active",
     }

src/phylum/ci/ci_base.py

@@ -4,22 +4,22 @@
 integration (CI) environment. Common functionality is provided where possible and CI specific features are
 designated as abstract methods to be defined in specific CI environments.
 """
+from abc import ABC, abstractmethod
+from argparse import Namespace
+from collections import OrderedDict
+from functools import cached_property
 import json
 import os
+from pathlib import Path
 import shlex
 import shutil
 import subprocess
 import tempfile
 import textwrap
-from abc import ABC, abstractmethod
-from argparse import Namespace
-from collections import OrderedDict
-from functools import cached_property
-from pathlib import Path
 from typing import List, Optional
 
-import pathspec
 from packaging.version import Version
+import pathspec
 from rich.markdown import Markdown
 from ruamel.yaml import YAML
 
@@ -131,7 +131,7 @@ def filter_lockfiles(self, provided_lockfiles: List[Path]) -> Lockfiles:
             cmd = [str(self.cli_path), "parse", str(provided_lockfile)]
             # stdout and stderr are piped to DEVNULL because we only care about the return code.
             # pylint: disable-next=subprocess-run-check ; we want return code here and don't want to raise when non-zero
-            if bool(subprocess.run(cmd, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL).returncode):
+            if bool(subprocess.run(cmd, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL).returncode):  # noqa: S603
                 print(f" [!] Provided lockfile failed to parse as a known lockfile type: {provided_lockfile}")
                 continue
             lockfiles.append(Lockfile(provided_lockfile, self.cli_path, self.common_ancestor_commit))
@@ -235,7 +235,7 @@ def cli_path(self) -> Path:
         # Ensure stdout is piped to DEVNULL, to keep the token from being printed in (CI log) output.
         cmd = [str(cli_path), "auth", "token"]
         # pylint: disable-next=subprocess-run-check ; we want the return code here and don't want to raise when non-zero
-        if bool(subprocess.run(cmd, stdout=subprocess.DEVNULL).returncode):
+        if bool(subprocess.run(cmd, stdout=subprocess.DEVNULL).returncode):  # noqa: S603
             raise SystemExit(" [!] A Phylum API key is required to continue.")
 
         print(f" [+] Using Phylum CLI instance: {cli_version} at {cli_path}")
@@ -256,7 +256,7 @@ def phylum_label(self) -> str:
           * Start the label with the `self.ci_platform_name`
           * Replace all runs of whitespace characters with a single `-` character
         """
-        raise NotImplementedError()
+        raise NotImplementedError
 
     @property
     def lockfile_hash_object(self) -> str:
@@ -285,7 +285,7 @@ def common_ancestor_commit(self) -> Optional[str]:
         When found, it should be returned as a string of the SHA1 sum representing the commit.
         When it can't be found (or there is an error), `None` should be returned.
         """
-        raise NotImplementedError()
+        raise NotImplementedError
 
     @property
     @abstractmethod
@@ -294,7 +294,7 @@ def is_any_lockfile_changed(self) -> bool:
 
         Implementations should return `True` if any lockfile has changed and `False` otherwise.
         """
-        raise NotImplementedError()
+        raise NotImplementedError
 
     def update_lockfiles_change_status(self, commit: str, err_msg: Optional[str] = None) -> None:
         """Update each lockfile's change status.
@@ -308,7 +308,7 @@ def update_lockfiles_change_status(self, commit: str, err_msg: Optional[str] = N
             # `--exit-code` will make git exit with 1 if there were differences while 0 means no differences.
             # Any other exit code is an error and a reason to re-raise.
             cmd = ["git", "diff", "--exit-code", "--quiet", commit, "--", str(lockfile.path)]
-            ret = subprocess.run(cmd, check=False)
+            ret = subprocess.run(cmd, check=False)  # noqa: S603
             if ret.returncode == 0:
                 print(f" [-] The lockfile `{lockfile!r}` has not changed")
                 lockfile.is_lockfile_changed = False
@@ -349,15 +349,16 @@ def ensure_project_exists(self) -> None:
         if self.phylum_group:
             print(f" [-] Using Phylum group: {self.phylum_group}")
             cmd = [str(self.cli_path), "project", "create", "--group", self.phylum_group, self.phylum_project]
-        ret = subprocess.run(cmd, check=False, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
+        ret = subprocess.run(cmd, check=False, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)  # noqa: S603
         # The Phylum CLI will return a unique error code of 14 when a project that already
         # exists is attempted to be created. This situation is recognized and allowed to happen
         # since it means the project exists as expected. Any other exit code is an error.
+        cli_exit_code_project_already_exists = 14
         if ret.returncode == 0:
             print(f" [-] Project {self.phylum_project} created successfully.")
             if self._project_file_already_existed:
                 print(f" [!] Overwrote previous `.phylum_project` file found at: {self._phylum_project_file}")
-        elif ret.returncode == 14:
+        elif ret.returncode == cli_exit_code_project_already_exists:
             print(f" [-] Project {self.phylum_project} already exists. Continuing with it ...")
         else:
             print(f" [!] There was a problem creating the project with command: {shlex.join(cmd)}")
@@ -407,11 +408,12 @@ def analyze(self) -> ReturnCode:
 
             print(f" [*] Performing analysis with command: {shlex.join(cmd)}")
             try:
-                analysis_result = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout
+                analysis_result = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout  # noqa: S603
             except subprocess.CalledProcessError as err:
                 # The Phylum project will fail analysis if the configured policy criteria are not met.
                 # This causes the return code to be 100 and lands us here. Check for this case to proceed.
-                if err.returncode == 100:
+                cli_exit_code_failed_policy = 100
+                if err.returncode == cli_exit_code_failed_policy:
                     analysis_result = err.stdout
                 else:
                     print(f" [!] stdout:\n{err.stdout}")
@@ -430,13 +432,12 @@ def analyze(self) -> ReturnCode:
             else:
                 print(" [+] The analysis is complete and there were NO failures")
                 returncode = ReturnCode.SUCCESS
+        elif analysis.is_failure:
+            print(" [!] There were failures in one or more completed packages")
+            returncode = ReturnCode.FAILURE_INCOMPLETE
         else:
-            if analysis.is_failure:
-                print(" [!] There were failures in one or more completed packages")
-                returncode = ReturnCode.FAILURE_INCOMPLETE
-            else:
-                print(" [+] There were no failures in the packages that have completed so far")
-                returncode = ReturnCode.INCOMPLETE
+            print(" [+] There were no failures in the packages that have completed so far")
+            returncode = ReturnCode.INCOMPLETE
 
         return returncode
 

src/phylum/ci/ci_bitbucket.py

@@ -16,14 +16,14 @@
   * https://developer.atlassian.com/cloud/bitbucket/rest/api-group-pullrequests/
   * https://developer.atlassian.com/cloud/bitbucket/rest/intro/#pullrequest
 """
+from argparse import Namespace
+from functools import cached_property, lru_cache
 import os
 import re
 import shlex
 import subprocess
-import urllib.parse
-from argparse import Namespace
-from functools import cached_property, lru_cache
 from typing import Optional
+import urllib.parse
 
 import requests
 
@@ -61,7 +61,7 @@ def is_in_pr() -> bool:
 class CIBitbucket(CIBase):
     """Provide methods for a Bitbucket Pipelines environment."""
 
-    def __init__(self, args: Namespace) -> None:
+    def __init__(self, args: Namespace) -> None:  # noqa: D107 ; the base __init__ docstring is better here
         super().__init__(args)
         self.ci_platform_name = "Bitbucket Pipelines"
         if is_in_pr():
@@ -167,7 +167,7 @@ def common_ancestor_commit(self) -> Optional[str]:
         cmd = ["git", "merge-base", src_branch, tgt_branch]
         print(f" [*] Finding common ancestor commit with command: {shlex.join(cmd)}")
         try:
-            common_commit = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout.strip()
+            common_commit = subprocess.run(cmd, check=True, capture_output=True, text=True).stdout.strip()  # noqa: S603
         except subprocess.CalledProcessError as err:
             ref_url = "https://support.atlassian.com/bitbucket-cloud/docs/git-clone-behavior/"
             print(f" [!] The common ancestor commit could not be found: {err}")