Repository: pypa/pip
Pull request #10956 (2022-03-10 11:48:25)

More structured link evaluation result
Follow-up refactoring to #9708.

A new enum class is implemented for the link evaluator to use instead of a simple boolean to better distinguish between various evaluation errors. This allows the caller to better distinguish error sources with a structured check instead of fragile error string comparison.



Patch:
diff --git a/src/pip/_internal/index/package_finder.py b/src/pip/_internal/index/package_finder.py
index 2dfc03c36..f70f74b17 100644
--- a/src/pip/_internal/index/package_finder.py
+++ b/src/pip/_internal/index/package_finder.py
@@ -3,6 +3,7 @@
 # The following comment should be removed at some point in the future.
 # mypy: strict-optional=False
 
+import enum
 import functools
 import itertools
 import logging
@@ -94,6 +95,16 @@ def _check_link_requires_python(
     return True
 
 
+class LinkType(enum.Enum):
+    candidate = enum.auto()
+    different_project = enum.auto()
+    yanked = enum.auto()
+    format_unsupported = enum.auto()
+    format_invalid = enum.auto()
+    platform_mismatch = enum.auto()
+    requires_python_mismatch = enum.auto()
+
+
 class LinkEvaluator:
 
     """
@@ -143,19 +154,20 @@ class LinkEvaluator:
 
         self.project_name = project_name
 
-    def evaluate_link(self, link: Link) -> Tuple[bool, Optional[str]]:
+    def evaluate_link(self, link: Link) -> Tuple[LinkType, str]:
         """
         Determine whether a link is a candidate for installation.
 
-        :return: A tuple (is_candidate, result), where `result` is (1) a
-            version string if `is_candidate` is True, and (2) if
-            `is_candidate` is False, an optional string to log the reason
-            the link fails to qualify.
+        :return: A tuple (result, detail), where *result* is an enum
+            representing whether the evaluation found a candidate, or the reason
+            why one is not found. If a candidate is found, *detail* will be the
+            candidate's version string; if one is not found, it contains the
+            reason the link fails to qualify.
         """
         version = None
         if link.is_yanked and not self._allow_yanked:
             reason = link.yanked_reason or "<none given>"
-            return (False, f"yanked for reason: {reason}")
+            return (LinkType.yanked, f"yanked for reason: {reason}")
 
         if link.egg_fragment:
             egg_info = link.egg_fragment
@@ -163,42 +175,46 @@ class LinkEvaluator:
         else:
             egg_info, ext = link.splitext()
             if not ext:
-                return (False, "not a file")
+                return (LinkType.format_unsupported, "not a file")
             if ext not in SUPPORTED_EXTENSIONS:
-                return (False, f"unsupported archive format: {ext}")
+                return (
+                    LinkType.format_unsupported,
+                    f"unsupported archive format: {ext}",
+                )
             if "binary" not in self._formats and ext == WHEEL_EXTENSION:
-                reason = "No binaries permitted for {}".format(self.project_name)
-                return (False, reason)
+                reason = f"No binaries permitted for {self.project_name}"
+                return (LinkType.format_unsupported, reason)
             if "macosx10" in link.path and ext == ".zip":
-                return (False, "macosx10 one")
+                return (LinkType.format_unsupported, "macosx10 one")
             if ext == WHEEL_EXTENSION:
                 try:
                     wheel = Wheel(link.filename)
                 except InvalidWheelFilename:
-                    return (False, "invalid wheel filename")
+                    return (
+                        LinkType.format_invalid,
+                        "invalid wheel filename",
+                    )
                 if canonicalize_name(wheel.name) != self._canonical_name:
-                    reason = "wrong project name (not {})".format(self.project_name)
-                    return (False, reason)
+                    reason = f"wrong project name (not {self.project_name})"
+                    return (LinkType.different_project, reason)
 
                 supported_tags = self._target_python.get_tags()
                 if not wheel.supported(supported_tags):
                     # Include the wheel's tags in the reason string to
                     # simplify troubleshooting compatibility issues.
-                    file_tags = wheel.get_formatted_file_tags()
+                    file_tags = ", ".join(wheel.get_formatted_file_tags())
                     reason = (
-                        "none of the wheel's tags ({}) are compatible "
-                        "(run pip debug --verbose to show compatible tags)".format(
-                            ", ".join(file_tags)
-                        )
+                        f"none of the wheel's tags ({file_tags}) are compatible "
+                        f"(run pip debug --verbose to show compatible tags)"
                     )
-                    return (False, reason)
+                    return (LinkType.platform_mismatch, reason)
 
                 version = wheel.version
 
         # This should be up by the self.ok_binary check, but see issue 2700.
         if "source" not in self._formats and ext != WHEEL_EXTENSION:
             reason = f"No sources permitted for {self.project_name}"
-            return (False, reason)
+            return (LinkType.format_unsupported, reason)
 
         if not version:
             version = _extract_version_from_fragment(
@@ -207,14 +223,17 @@ class LinkEvaluator:
             )
         if not version:
             reason = f"Missing project version for {self.project_name}"
-            return (False, reason)
+            return (LinkType.format_invalid, reason)
 
         match = self._py_version_re.search(version)
         if match:
             version = version[: match.start()]
             py_version = match.group(1)
             if py_version != self._target_python.py_version:
-                return (False, "Python version is incorrect")
+                return (
+                    LinkType.platform_mismatch,
+                    "Python version is incorrect",
+                )
 
         supports_python = _check_link_requires_python(
             link,
@@ -223,11 +242,11 @@ class LinkEvaluator:
         )
         if not supports_python:
             reason = f"{version} Requires-Python {link.requires_python}"
-            return (False, reason)
+            return (LinkType.requires_python_mismatch, reason)
 
         logger.debug("Found link %s, version: %s", link, version)
 
-        return (True, version)
+        return (LinkType.candidate, version)
 
 
 def filter_unallowed_hashes(
@@ -609,7 +628,7 @@ class PackageFinder:
         self.format_control = format_control
 
         # These are boring links that have already been logged somehow.
-        self._logged_links: Set[Tuple[Link, str]] = set()
+        self._logged_links: Set[Tuple[Link, LinkType, str]] = set()
 
     # Don't include an allow_yanked default value to make sure each call
     # site considers whether yanked releases are allowed. This also causes
@@ -689,10 +708,13 @@ class PackageFinder:
     def set_prefer_binary(self) -> None:
         self._candidate_prefs.prefer_binary = True
 
-    def skipped_links_requires_python(self) -> List[str]:
-        return sorted(
-            {reason for _, reason in self._logged_links if "Requires-Python" in reason}
-        )
+    def requires_python_skipped_reasons(self) -> List[str]:
+        reasons = {
+            detail
+            for _, result, detail in self._logged_links
+            if result == LinkType.requires_python_mismatch
+        }
+        return sorted(reasons)
 
     def make_link_evaluator(self, project_name: str) -> LinkEvaluator:
         canonical_name = canonicalize_name(project_name)
@@ -723,12 +745,13 @@ class PackageFinder:
                     no_eggs.append(link)
         return no_eggs + eggs
 
-    def _log_skipped_link(self, link: Link, reason: str) -> None:
-        if (link, reason) not in self._logged_links:
+    def _log_skipped_link(self, link: Link, result: LinkType, detail: str) -> None:
+        entry = (link, result, detail)
+        if entry not in self._logged_links:
             # Put the link at the end so the reason is more visible and because
             # the link string is usually very long.
-            logger.debug("Skipping link: %s: %s", reason, link)
-            self._logged_links.add((link, reason))
+            logger.debug("Skipping link: %s: %s", detail, link)
+            self._logged_links.add(entry)
 
     def get_install_candidate(
         self, link_evaluator: LinkEvaluator, link: Link
@@ -737,15 +760,15 @@ class PackageFinder:
         If the link is a candidate for install, convert it to an
         InstallationCandidate and return it. Otherwise, return None.
         """
-        is_candidate, result = link_evaluator.evaluate_link(link)
-        if not is_candidate:
-            self._log_skipped_link(link, result)
+        result, detail = link_evaluator.evaluate_link(link)
+        if result != LinkType.candidate:
+            self._log_skipped_link(link, result, detail)
             return None
 
         return InstallationCandidate(
             name=link_evaluator.project_name,
             link=link,
-            version=result,
+            version=detail,
         )
 
     def evaluate_links(
diff --git a/src/pip/_internal/resolution/resolvelib/factory.py b/src/pip/_internal/resolution/resolvelib/factory.py
index d3648e158..4569033db 100644
--- a/src/pip/_internal/resolution/resolvelib/factory.py
+++ b/src/pip/_internal/resolution/resolvelib/factory.py
@@ -617,14 +617,14 @@ class Factory:
             req_disp = f"{req} (from {parent.name})"
 
         cands = self._finder.find_all_candidates(req.project_name)
-        skips = self._finder.skipped_links_requires_python()
+        skipped_by_requires_python = self._finder.requires_python_skipped_reasons()
         versions = [str(v) for v in sorted({c.version for c in cands})]
 
-        if skips:
+        if skipped_by_requires_python:
             logger.critical(
                 "Ignored the following versions that require a different python "
                 "version: %s",
-                "; ".join(skips) or "none",
+                "; ".join(skipped_by_requires_python) or "none",
             )
         logger.critical(
             "Could not find a version that satisfies the requirement %s "


Test patch:
diff --git a/tests/unit/test_finder.py b/tests/unit/test_finder.py
index deff29582..16c240144 100644
--- a/tests/unit/test_finder.py
+++ b/tests/unit/test_finder.py
@@ -14,6 +14,7 @@ from pip._internal.index.package_finder import (
     InstallationCandidate,
     Link,
     LinkEvaluator,
+    LinkType,
 )
 from pip._internal.models.target_python import TargetPython
 from pip._internal.req.constructors import install_req_from_line
@@ -501,26 +502,36 @@ class TestLinkEvaluator:
         link = Link(url)
         evaluator = self.make_test_link_evaluator(formats=["source", "binary"])
         actual = evaluator.evaluate_link(link)
-        assert actual == (True, expected_version)
+        assert actual == (LinkType.candidate, expected_version)
 
     @pytest.mark.parametrize(
-        "url, expected_msg",
+        "url, link_type, fail_reason",
         [
             # TODO: Uncomment this test case when #1217 is fixed.
             # 'http:/yo/pytest-xdist-1.0.tar.gz',
-            ("http:/yo/pytest2-1.0.tar.gz", "Missing project version for pytest"),
+            (
+                "http:/yo/pytest2-1.0.tar.gz",
+                LinkType.format_invalid,
+                "Missing project version for pytest",
+            ),
             (
                 "http:/yo/pytest_xdist-1.0-py2.py3-none-any.whl",
+                LinkType.different_project,
                 "wrong project name (not pytest)",
             ),
         ],
     )
-    def test_evaluate_link__substring_fails(self, url: str, expected_msg: str) -> None:
+    def test_evaluate_link__substring_fails(
+        self,
+        url: str,
+        link_type: LinkType,
+        fail_reason: str,
+    ) -> None:
         """Test that 'pytest<something> archives won't match for 'pytest'."""
         link = Link(url)
         evaluator = self.make_test_link_evaluator(formats=["source", "binary"])
         actual = evaluator.evaluate_link(link)
-        assert actual == (False, expected_msg)
+        assert actual == (link_type, fail_reason)
 
 
 def test_process_project_url(data: TestData) -> None:
diff --git a/tests/unit/test_index.py b/tests/unit/test_index.py
index 7fbcd3766..705126854 100644
--- a/tests/unit/test_index.py
+++ b/tests/unit/test_index.py
@@ -11,6 +11,7 @@ from pip._internal.index.package_finder import (
     CandidatePreferences,
     FormatControl,
     LinkEvaluator,
+    LinkType,
     PackageFinder,
     _check_link_requires_python,
     _extract_version_from_fragment,
@@ -116,20 +117,36 @@ def test_check_link_requires_python__invalid_requires(
 
 class TestLinkEvaluator:
     @pytest.mark.parametrize(
-        "py_version_info,ignore_requires_python,expected",
+        "py_version_info, ignore_requires_python, expected",
         [
-            ((3, 6, 5), False, (True, "1.12")),
-            # Test an incompatible Python.
-            ((3, 6, 4), False, (False, "1.12 Requires-Python == 3.6.5")),
-            # Test an incompatible Python with ignore_requires_python=True.
-            ((3, 6, 4), True, (True, "1.12")),
+            pytest.param(
+                (3, 6, 5),
+                False,
+                (LinkType.candidate, "1.12"),
+                id="compatible",
+            ),
+            pytest.param(
+                (3, 6, 4),
+                False,
+                (
+                    LinkType.requires_python_mismatch,
+                    "1.12 Requires-Python == 3.6.5",
+                ),
+                id="requires-python-mismatch",
+            ),
+            pytest.param(
+                (3, 6, 4),
+                True,
+                (LinkType.candidate, "1.12"),
+                id="requires-python-mismatch-ignored",
+            ),
         ],
     )
     def test_evaluate_link(
         self,
         py_version_info: Tuple[int, int, int],
         ignore_requires_python: bool,
-        expected: Tuple[bool, Optional[str]],
+        expected: Tuple[LinkType, str],
     ) -> None:
         target_python = TargetPython(py_version_info=py_version_info)
         evaluator = LinkEvaluator(
@@ -150,18 +167,29 @@ class TestLinkEvaluator:
     @pytest.mark.parametrize(
         "yanked_reason, allow_yanked, expected",
         [
-            (None, True, (True, "1.12")),
-            (None, False, (True, "1.12")),
-            ("", True, (True, "1.12")),
-            ("", False, (False, "yanked for reason: <none given>")),
-            ("bad metadata", True, (True, "1.12")),
-            ("bad metadata", False, (False, "yanked for reason: bad metadata")),
+            (None, True, (LinkType.candidate, "1.12")),
+            (None, False, (LinkType.candidate, "1.12")),
+            ("", True, (LinkType.candidate, "1.12")),
+            (
+                "",
+                False,
+                (LinkType.yanked, "yanked for reason: <none given>"),
+            ),
+            ("bad metadata", True, (LinkType.candidate, "1.12")),
+            (
+                "bad metadata",
+                False,
+                (LinkType.yanked, "yanked for reason: bad metadata"),
+            ),
             # Test a unicode string with a non-ascii character.
-            ("curly quote: \u2018", True, (True, "1.12")),
+            ("curly quote: \u2018", True, (LinkType.candidate, "1.12")),
             (
                 "curly quote: \u2018",
                 False,
-                (False, "yanked for reason: curly quote: \u2018"),
+                (
+                    LinkType.yanked,
+                    "yanked for reason: curly quote: \u2018",
+                ),
             ),
         ],
     )
@@ -169,7 +197,7 @@ class TestLinkEvaluator:
         self,
         yanked_reason: str,
         allow_yanked: bool,
-        expected: Tuple[bool, str],
+        expected: Tuple[LinkType, str],
     ) -> None:
         target_python = TargetPython(py_version_info=(3, 6, 4))
         evaluator = LinkEvaluator(
@@ -203,7 +231,7 @@ class TestLinkEvaluator:
         link = Link("https://example.com/sample-1.0-py2.py3-none-any.whl")
         actual = evaluator.evaluate_link(link)
         expected = (
-            False,
+            LinkType.platform_mismatch,
             "none of the wheel's tags (py2-none-any, py3-none-any) are compatible "
             "(run pip debug --verbose to show compatible tags)",
         )