Handle package-specifier spacing (#197)

Use the spacing between package name and specifier. New: - Add `--verbose` option to update-deps. - Add `debug` input for CI - Check dependencies. Log sub-line input to update_file() in update_deps(). Update tests to include spacing issues. Add test for the --verbose flag. Add comments in test according to code review. Avoid `name3` in test since multiple version specifiers are not reliably supported. Follow issue #141 for updates. Explain change in `name2`. Co-authored-by: Anders Eklund <[email protected]>
SINTEF · Oct 25, 2023 · a33be22 · a33be22
1 parent 3f0821e
commit a33be22
Show file tree

Hide file tree

Showing 5 changed files with 149 additions and 30 deletions.
diff --git a/.github/workflows/ci_check_pyproject_dependencies.yml b/.github/workflows/ci_check_pyproject_dependencies.yml
@@ -51,6 +51,11 @@ on:
         required: false
         type: string
         default: ""
+      debug:
+        description: "Whether to run the workflow in debug mode, printing extra debug information."
+        required: false
+        type: boolean
+        default: false
     secrets:
       PAT:
         description: "A personal access token (PAT) with rights to update the `permanent_dependencies_branch`. This will fallback on `GITHUB_TOKEN`."
@@ -75,18 +80,36 @@ jobs:
 
     - name: Install Python dependencies
       run: |
-        python -m pip install -U pip
-        pip install -U setuptools wheel
-        pip install .${{ inputs.install_extras }}
-        pip install git+https://github.com/SINTEF/[email protected]
+        if [ "${{ inputs.debug }}" == "true" ]; then
+          set -x
+          VERBOSE=-vvv
+        else
+          VERBOSE=
+        fi
+
+        python -m pip install -U ${VERBOSE} pip
+        pip install -U ${VERBOSE} setuptools wheel
+        pip install ${VERBOSE} .${{ inputs.install_extras }}
+        pip install ${VERBOSE} git+https://github.com/SINTEF/[email protected]
 
     - name: Set up git user
       run: |
+        if [ "${{ inputs.debug }}" == "true" ]; then
+          set -x
+        fi
+
         git config --global user.name "${{ inputs.git_username }}"
         git config --global user.email "${{ inputs.git_email }}"
 
     - name: Run ci-cd task
       run: |
+        if [ "${{ inputs.debug }}" == "true" ]; then
+          set -x
+          VERBOSE=--verbose
+        else
+          VERBOSE=
+        fi
+
         if [ "${{ inputs.fail_fast }}" == "true" ]; then
           FAIL_FAST=--fail-fast
         else
@@ -98,10 +121,15 @@ jobs:
           if [ -n "${line}" ]; then IGNORE_OPTIONS+=(--ignore="${line}"); fi
         done <<< "${{ inputs.ignore }}"
 
-        ci-cd update-deps ${FAIL_FAST} \
+        ci-cd update-deps ${FAIL_FAST} ${VERBOSE} \
           --root-repo-path="${PWD}" \
+          --ignore-separator="..." \
           "${IGNORE_OPTIONS[@]}"
 
+        if [ "${{ inputs.debug }}" == "true" ]; then
+          git status
+        fi
+
         if [ -n "$(git status --porcelain pyproject.toml)" ]; then
           echo "UPDATE_DEPS=true" >> $GITHUB_ENV
           git add pyproject.toml
@@ -113,6 +141,10 @@ jobs:
     - name: Set PR body
       if: env.UPDATE_DEPS == 'true'
       run: |
+        if [ "${{ inputs.debug }}" == "true" ]; then
+          set -x
+        fi
+
         if [ -z "${{ inputs.pr_body_file }}" ] || [ ! -f "${{ inputs.pr_body_file }}" ]; then
           PR_BODY_FILE=.tmp_pr-body_${{ github.run_id }}_${{ github.run_number }}_${{ github.run_attempt }}.txt
           echo "PR_BODY_FILE=${PR_BODY_FILE}" >> $GITHUB_ENV

diff --git a/ci_cd/tasks/update_deps.py b/ci_cd/tasks/update_deps.py
@@ -52,6 +52,7 @@
             "Value to use instead of ellipsis (`...`) as a separator in `--ignore` "
             "key/value-pairs."
         ),
+        "verbose": "Whether or not to print debug statements.",
     },
     iterable=["ignore"],
 )
@@ -62,6 +63,7 @@ def update_deps(  # pylint: disable=too-many-branches,too-many-locals,too-many-s
     pre_commit=False,
     ignore=None,
     ignore_separator="...",
+    verbose=False,
 ):
     """Update dependencies in specified Python package's `pyproject.toml`."""
     if TYPE_CHECKING:  # pragma: no cover
@@ -70,10 +72,16 @@ def update_deps(  # pylint: disable=too-many-branches,too-many-locals,too-many-s
         fail_fast: bool = fail_fast  # type: ignore[no-redef]
         pre_commit: bool = pre_commit  # type: ignore[no-redef]
         ignore_separator: str = ignore_separator  # type: ignore[no-redef]
+        verbose: bool = verbose  # type: ignore[no-redef]
 
     if not ignore:
         ignore: list[str] = []  # type: ignore[no-redef]
 
+    if verbose:
+        LOGGER.setLevel(logging.DEBUG)
+        LOGGER.addHandler(logging.StreamHandler(sys.stdout))
+        LOGGER.debug("Verbose logging enabled.")
+
     VersionSpec = namedtuple(
         "VersionSpec",
         [
@@ -84,6 +92,7 @@ def update_deps(  # pylint: disable=too-many-branches,too-many-locals,too-many-s
             "version",
             "extra_operator_version",
             "environment_marker",
+            "spacing",
         ],
     )
 
@@ -134,7 +143,8 @@ def update_deps(  # pylint: disable=too-many-branches,too-many-locals,too-many-s
     error = False
     for line in dependencies:
         match = re.match(
-            r"^(?P<full_dependency>(?P<package>[a-zA-Z0-9_.-]+)(?:\s*\[.*\])?)\s*"
+            r"^(?P<full_dependency>(?P<package>[a-zA-Z0-9_.-]+)(?:\s*\[.*\])?)"
+            r"(?P<spacing>\s*)"
             r"(?:"
             r"(?P<url_version>@\s*\S+)|"
             r"(?P<operator>>|<|<=|>=|==|!=|~=)\s*"
@@ -267,16 +277,27 @@ def update_deps(  # pylint: disable=too-many-branches,too-many-locals,too-many-s
             escaped_full_dependency_name = version_spec.full_dependency.replace(
                 "[", r"\["
             ).replace("]", r"\]")
-            update_file(
-                pyproject_path,
-                (
-                    rf'"{escaped_full_dependency_name} {version_spec.operator}.*"',
-                    f'"{version_spec.full_dependency} '
-                    f"{version_spec.operator}{updated_version}"
-                    f'{version_spec.extra_operator_version if version_spec.extra_operator_version else ""}'  # pylint: disable=line-too-long
-                    f'{version_spec.environment_marker if version_spec.environment_marker else ""}"',  # pylint: disable=line-too-long
-                ),
+
+            LOGGER.debug("updated_version: %s", updated_version)
+            LOGGER.debug(
+                "escaped_full_dependency_name: %s", escaped_full_dependency_name
+            )
+
+            pattern_sub_line = (
+                rf'"{escaped_full_dependency_name}{version_spec.spacing}'
+                rf'{version_spec.operator}.*"'
             )
+            replacement_sub_line = (
+                f'"{version_spec.full_dependency}{version_spec.spacing}'
+                f"{version_spec.operator}{updated_version}"
+                f'{version_spec.extra_operator_version if version_spec.extra_operator_version else ""}'  # pylint: disable=line-too-long
+                f'{version_spec.environment_marker if version_spec.environment_marker else ""}"'  # pylint: disable=line-too-long
+            )
+
+            LOGGER.debug("pattern_sub_line: %s", pattern_sub_line)
+            LOGGER.debug("replacement_sub_line: %s", replacement_sub_line)
+
+            update_file(pyproject_path, (pattern_sub_line, replacement_sub_line))
             already_handled_packages.add(version_spec.package)
             updated_packages[version_spec.full_dependency] = (
                 f"{version_spec.operator}{updated_version}"

diff --git a/docs/hooks/update_pyproject.md b/docs/hooks/update_pyproject.md
@@ -55,6 +55,7 @@ Any of these options can be given through the `args` key when defining the hook.
 | `--fail-fast` | Fail immediately if an error occurs. Otherwise, print and ignore all non-critical errors. | No | `False` | _boolean_ |
 | `--ignore` | Ignore-rules based on the `ignore` config option of Dependabot.</br></br>It should be of the format: `key=value...key=value`, i.e., an ellipsis (`...`) separator and then equal-sign-separated key/value-pairs.</br>Alternatively, the `--ignore-separator` can be set to something else to overwrite the ellipsis.</br></br>The only supported keys are: `dependency-name`, `versions`, and `update-types`.</br></br>Can be supplied multiple times per `dependency-name`. | No | | _string_ |
 | `--ignore-separator` | Value to use instead of ellipsis (`...`) as a separator in `--ignore` key/value-pairs. | No | | _string_ |
+| `--verbose` | Whether or not to print debug statements. | No | `False` | _boolean_ |
 
 ## Usage example
 

diff --git a/docs/workflows/ci_check_pyproject_dependencies.md b/docs/workflows/ci_check_pyproject_dependencies.md
@@ -70,6 +70,7 @@ The repository contains the following:
 | `pr_labels` | A comma separated list of strings of GitHub labels to use for the created PR. | No | _Empty string_ | _string_ |
 | `ignore` | Create ignore conditions for certain dependencies. A multi-line string of ignore rules, where each line is an ellipsis-separated (`...`) string of key/value-pairs. One line per dependency. This option is similar to [the `ignore` option of Dependabot](https://docs.github.com/en/code-security/dependabot/dependabot-version-updates/configuration-options-for-the-dependabot.yml-file#ignore).</br></br>See also [Single vs multi-line input](index.md#single-vs-multi-line-input). | No | _Empty string_ | _string_
 | `branch_name_extension` | A string to append to the branch name of the created PR. Example: `'-my-branch'`. It will be appended after a forward slash, so the final branch name will be `ci/update-pyproject/-my-branch`. | No | _Empty string_ | _string_ |
+| `debug` | Whether to run the workflow in debug mode, printing extra debug information. | No | `false` | _boolean_ |
 
 ## Secrets
 

diff --git a/tests/tasks/test_update_deps.py b/tests/tasks/test_update_deps.py
@@ -52,7 +52,7 @@ def test_update_deps(tmp_path: "Path", caplog: pytest.LogCaptureFixture) -> None
 ]
 dev = [
     "mike >={original_dependencies['mike']},<3",
-    "pre-commit ~={original_dependencies['pre-commit']}",
+    "pre-commit~={original_dependencies['pre-commit']}",
     "pylint ~={original_dependencies['pylint']}",
     "test[testing]",
 ]
@@ -65,7 +65,9 @@ def test_update_deps(tmp_path: "Path", caplog: pytest.LogCaptureFixture) -> None
     "name",
     "name1<=1",
     "name2>=3",
-    "name3>=3,<2",
+    # Multiple version specifiers are currently not supported.
+    # Follow issue #141 for updates.
+    # "name3>=3,<2",
     "name4@http://foo.com",
     "name5 [fred,bar] @ http://foo.com ; python_version=='2.7'",
     "name6[quux, strange];python_version<'2.7' and platform_version=='2'",
@@ -117,11 +119,12 @@ def test_update_deps(tmp_path: "Path", caplog: pytest.LogCaptureFixture) -> None
 
     for line in dependencies:
         if any(
-            line.startswith(package_name)
-            for package_name in ["invoke ", "pytest ", "pre-commit "]
+            line.startswith(package_name) for package_name in ["invoke ", "pytest "]
         ):
             package_name = line.split(maxsplit=1)[0]
             assert line == f"{package_name} ~={original_dependencies[package_name]}"
+        elif "pre-commit" in line:
+            assert line == f"pre-commit~={original_dependencies['pre-commit']}"
         elif "tomlkit" in line:
             # Should be three version digits, since the original dependency had three.
             assert line == "tomlkit[test,docs] ~=1.0.0"
@@ -149,9 +152,19 @@ def test_update_deps(tmp_path: "Path", caplog: pytest.LogCaptureFixture) -> None
         elif "name1" in line:
             assert line == "name1<=1"
         elif "name2" in line:
-            assert line == "name2>=3"
+            # name2 is altered by update_deps() to include the latest version (v1.2.3)
+            # even though it was originally specifying all versions above and including
+            # v3.
+            # Note, if a user wishes to avoid this alteration, the `--ignore` option can
+            # be used to specify the package (and version(s)) to ignore.
+            assert line == "name2>=1"
         elif "name3" in line:
-            assert line == "name3>=3,<2"
+            # Multiple version specifiers are currently not supported.
+            # Follow issue #141 for updates.
+            pytest.fail(
+                "name3 is commnted out in the test file and should not be present."
+            )
+            # assert line == "name3>=3,<2"
         elif "name4" in line:
             assert line == "name4@http://foo.com"
             assert "'name4' is pinned to a URL and will be skipped" in caplog.text
@@ -983,7 +996,7 @@ def test_ignore_version_fails() -> None:
                 "mike": "mike >=1.0,<3",
                 "pytest": "pytest ~=7.2",
                 "pytest-cov": "pytest-cov ~=3.1",
-                "pre-commit": "pre-commit ~=2.20",
+                "pre-commit": "pre-commit~=2.20",
                 "pylint": "pylint ~=2.14",
                 "Sphinx": "Sphinx >=4.5.0,<6",
             },
@@ -996,7 +1009,7 @@ def test_ignore_version_fails() -> None:
                 "mike": "mike >=1.0,<3",
                 "pytest": "pytest ~=7.2",
                 "pytest-cov": "pytest-cov ~=3.1",
-                "pre-commit": "pre-commit ~=2.20",
+                "pre-commit": "pre-commit~=2.20",
                 "pylint": "pylint ~=2.14",
                 "Sphinx": "Sphinx >=6.1.3,<6",
             },
@@ -1012,7 +1025,7 @@ def test_ignore_version_fails() -> None:
                 "mike": "mike >=1.0,<3",
                 "pytest": "pytest ~=7.2",
                 "pytest-cov": "pytest-cov ~=3.1",
-                "pre-commit": "pre-commit ~=2.20",
+                "pre-commit": "pre-commit~=2.20",
                 "pylint": "pylint ~=2.14",
                 "Sphinx": "Sphinx >=6.1.3,<6",
             },
@@ -1025,7 +1038,7 @@ def test_ignore_version_fails() -> None:
                 "mike": "mike >=1.0,<3",
                 "pytest": "pytest ~=7.2",
                 "pytest-cov": "pytest-cov ~=3.1",
-                "pre-commit": "pre-commit ~=2.20",
+                "pre-commit": "pre-commit~=2.20",
                 "pylint": "pylint ~=2.13",
                 "Sphinx": "Sphinx >=6.1.3,<6",
             },
@@ -1038,7 +1051,7 @@ def test_ignore_version_fails() -> None:
                 "mike": "mike >=1.0,<3",
                 "pytest": "pytest ~=7.1",
                 "pytest-cov": "pytest-cov ~=3.1",
-                "pre-commit": "pre-commit ~=2.20",
+                "pre-commit": "pre-commit~=2.20",
                 "pylint": "pylint ~=2.14",
                 "Sphinx": "Sphinx >=6.1.3,<6",
             },
@@ -1051,7 +1064,7 @@ def test_ignore_version_fails() -> None:
                 "mike": "mike >=1.0,<3",
                 "pytest": "pytest ~=7.2",
                 "pytest-cov": "pytest-cov ~=3.0",
-                "pre-commit": "pre-commit ~=2.20",
+                "pre-commit": "pre-commit~=2.20",
                 "pylint": "pylint ~=2.14",
                 "Sphinx": "Sphinx >=6.1.3,<6",  # This should be fixed!
             },
@@ -1064,7 +1077,7 @@ def test_ignore_version_fails() -> None:
                 "mike": "mike >=1.0,<3",
                 "pytest": "pytest ~=7.2",
                 "pytest-cov": "pytest-cov ~=3.1",
-                "pre-commit": "pre-commit ~=2.20",
+                "pre-commit": "pre-commit~=2.20",
                 "pylint": "pylint ~=2.14",
                 "Sphinx": "Sphinx >=4.5.0,<6",
             },
@@ -1127,7 +1140,7 @@ def test_ignore_rules_logic(
     "mike >={original_dependencies['mike']},<3",
     "pytest ~={original_dependencies['pytest']}",
     "pytest-cov ~={original_dependencies['pytest-cov']}",
-    "pre-commit ~={original_dependencies['pre-commit']}",
+    "pre-commit~={original_dependencies['pre-commit']}",
     "pylint ~={original_dependencies['pylint']}",
     "Sphinx >={original_dependencies['Sphinx']},<6",
 ]
@@ -1165,8 +1178,59 @@ def test_ignore_rules_logic(
 
     for line in dependencies:
         for dependency, dependency_requirement in expected_result.items():
-            if f"{dependency} " in line:
+            # Assert the dependency in the updated pyproject.toml file equals the
+            # expected value.
+            # We have to use a regular expression to match the dependency name as some
+            # dependency names are sub-strings of each other (like 'pytest' is a
+            # sub-string of 'pytest-cov').
+            if re.match(rf"{re.escape(dependency)}\s*(~|>).*", line):
                 assert line == dependency_requirement
                 break
         else:
             pytest.fail(f"Unknown package in line: {line}")
+
+
+@pytest.mark.parametrize("verbose_flag", [True, False])
+def test_verbose(
+    verbose_flag: bool,
+    tmp_path: Path,
+    capsys: pytest.CaptureFixture,
+    caplog: pytest.LogCaptureFixture,
+) -> None:
+    """Check the verbose flag is respected.
+
+    Any logged messaged should be written to stdout IF the verbose flag is set.
+    Check that any expected log messages are found both in the logs and in stdout.
+
+    If the verbose flag is not set, the messages should ONLY appear in the logs.
+    """
+    from invoke import MockContext
+
+    from ci_cd.tasks import update_deps
+
+    with pytest.raises(
+        SystemExit,
+        match=r".*Error: Could not find the Python package repository's 'pyproject.toml' file.*",
+    ):
+        update_deps(MockContext(), root_repo_path=str(tmp_path), verbose=verbose_flag)
+
+    captured_output = capsys.readouterr()
+
+    # Expected log messages - note the strings are sub-strings of the full log messages
+    assert (
+        "Verbose logging enabled." in caplog.text
+        if verbose_flag
+        else "Verbose logging enabled." not in caplog.text
+    )
+    assert "Parsed ignore rules:" in caplog.text
+
+    # Assert the above messages are the only messages in the logs
+    assert len(caplog.messages) == 2 if verbose_flag else 1
+
+    # Go through the log messages and ensure they either are or are not in stdout
+    for log_message in caplog.messages:
+        assert (
+            log_message in captured_output.out
+            if verbose_flag
+            else log_message not in captured_output.out
+        )