Merge pull request #10229 from pradyunsg/deprecation-cleanups

Give some TLC to the `deprecated` helper

Author: pradyunsg

src/pip/_internal/locations/__init__.py

@@ -315,10 +315,12 @@ def get_scheme(
     )
     if any(default_old[k] != getattr(old, k) for k in SCHEME_KEYS):
         deprecated(
-            "Configuring installation scheme with distutils config files "
-            "is deprecated and will no longer work in the near future. If you "
-            "are using a Homebrew or Linuxbrew Python, please see discussion "
-            "at https://github.com/Homebrew/homebrew-core/issues/76621",
+            reason=(
+                "Configuring installation scheme with distutils config files "
+                "is deprecated and will no longer work in the near future. If you "
+                "are using a Homebrew or Linuxbrew Python, please see discussion "
+                "at https://github.com/Homebrew/homebrew-core/issues/76621"
+            ),
             replacement=None,
             gone_in=None,
         )

src/pip/_internal/utils/deprecation.py

@@ -8,18 +8,9 @@
 
 from pip._vendor.packaging.version import parse
 
-from pip import __version__ as current_version
+from pip import __version__ as current_version  # NOTE: tests patch this name.
 
 DEPRECATION_MSG_PREFIX = "DEPRECATION: "
-DEPRECATION_MESSAGE = DEPRECATION_MSG_PREFIX + "{reason}"
-GONE_IN_MESSAGE_FUTURE = "pip {gone_in} will enforce this behavior change."
-GONE_IN_MESSAGE_PAST = "This behavior change has been enforced since pip {gone_in}."
-REPLACEMENT_MESSAGE = "A possible replacement is {replacement}."
-FEATURE_FLAG_MESSAGE = (
-    "You can temporarily use the flag --use-feature={feature_flag} "
-    "to test the upcoming behavior."
-)
-ISSUE_MESSAGE = "Discussion can be found at https://github.com/pypa/pip/issues/{issue}."
 
 
 class PipDeprecationWarning(Warning):
@@ -62,6 +53,7 @@ def install_warning_logger() -> None:
 
 
 def deprecated(
+    *,
     reason: str,
     replacement: Optional[str],
     gone_in: Optional[str],
@@ -86,42 +78,43 @@ def deprecated(
     issue:
         Issue number on the tracker that would serve as a useful place for
         users to find related discussion and provide feedback.
-
-    Always pass replacement, gone_in and issue as keyword arguments for clarity
-    at the call site.
     """
+
     # Determine whether or not the feature is already gone in this version.
     is_gone = gone_in is not None and parse(current_version) >= parse(gone_in)
-    # Allow variable substitutions within the "reason" variable.
-    formatted_reason = reason.format(gone_in=gone_in)
-    # Construct a nice message.
-    #   This is eagerly formatted as we want it to get logged as if someone
-    #   typed this entire message out.
-    formatted_deprecation_message = DEPRECATION_MESSAGE.format(reason=formatted_reason)
-    gone_in_message = GONE_IN_MESSAGE_PAST if is_gone else GONE_IN_MESSAGE_FUTURE
-    formatted_gone_in_message = (
-        gone_in_message.format(gone_in=gone_in) if gone_in else None
-    )
-    formatted_replacement_message = (
-        REPLACEMENT_MESSAGE.format(replacement=replacement) if replacement else None
-    )
-    formatted_feature_flag_message = (
-        None
-        if is_gone or not feature_flag
-        else FEATURE_FLAG_MESSAGE.format(feature_flag=feature_flag)
-    )
-    formatted_issue_message = ISSUE_MESSAGE.format(issue=issue) if issue else None
-    sentences = [
-        formatted_deprecation_message,
-        formatted_gone_in_message,
-        formatted_replacement_message,
-        formatted_feature_flag_message,
-        formatted_issue_message,
+
+    message_parts = [
+        (reason, f"{DEPRECATION_MSG_PREFIX}{{}}"),
+        (
+            gone_in,
+            "pip {} will enforce this behaviour change."
+            if not is_gone
+            else "Since pip {}, this is no longer supported.",
+        ),
+        (
+            replacement,
+            "A possible replacement is {}.",
+        ),
+        (
+            feature_flag,
+            "You can use the flag --use-feature={} to test the upcoming behaviour."
+            if not is_gone
+            else None,
+        ),
+        (
+            issue,
+            "Discussion can be found at https://github.com/pypa/pip/issues/{}",
+        ),
     ]
-    message = " ".join(sentence for sentence in sentences if sentence)
 
-    # Raise as an error if the functionality is gone.
+    message = " ".join(
+        format_str.format(value)
+        for value, format_str in message_parts
+        if format_str is not None and value is not None
+    )
+
+    # Raise as an error if this behaviour is deprecated.
     if is_gone:
         raise PipDeprecationWarning(message)
-    else:
-        warnings.warn(message, category=PipDeprecationWarning, stacklevel=2)
+
+    warnings.warn(message, category=PipDeprecationWarning, stacklevel=2)

tests/functional/test_warning.py

@@ -14,7 +14,7 @@ def warnings_demo(tmpdir):
         deprecation.install_warning_logger()
         basicConfig()
 
-        deprecation.deprecated("deprecated!", replacement=None, gone_in=None)
+        deprecation.deprecated(reason="deprecated!", replacement=None, gone_in=None)
     '''))
     return demo
 

tests/unit/test_utils.py

@@ -879,12 +879,16 @@ def patch_deprecation_check_version():
 @pytest.mark.parametrize("replacement", [None, "a magic 8 ball"])
 @pytest.mark.parametrize("gone_in", [None, "2.0"])
 @pytest.mark.parametrize("issue", [None, 988])
-def test_deprecated_message_contains_information(gone_in, replacement, issue):
+@pytest.mark.parametrize("feature_flag", [None, "magic-8-ball"])
+def test_deprecated_message_contains_information(
+    gone_in, replacement, issue, feature_flag
+):
     with pytest.warns(PipDeprecationWarning) as record:
         deprecated(
-            "Stop doing this!",
+            reason="Stop doing this!",
             replacement=replacement,
             gone_in=gone_in,
+            feature_flag=feature_flag,
             issue=issue,
         )
 
@@ -893,51 +897,77 @@ def test_deprecated_message_contains_information(gone_in, replacement, issue):
 
     assert "DEPRECATION: Stop doing this!" in message
     # Ensure non-None values are mentioned.
-    for item in [gone_in, replacement, issue]:
+    for item in [gone_in, replacement, issue, feature_flag]:
         if item is not None:
             assert str(item) in message
 
 
 @pytest.mark.usefixtures("patch_deprecation_check_version")
 @pytest.mark.parametrize("replacement", [None, "a magic 8 ball"])
 @pytest.mark.parametrize("issue", [None, 988])
-def test_deprecated_raises_error_if_too_old(replacement, issue):
+@pytest.mark.parametrize("feature_flag", [None, "magic-8-ball"])
+def test_deprecated_raises_error_if_too_old(replacement, issue, feature_flag):
     with pytest.raises(PipDeprecationWarning) as exception:
         deprecated(
-            "Stop doing this!",
+            reason="Stop doing this!",
             gone_in="1.0",  # this matches the patched version.
             replacement=replacement,
+            feature_flag=feature_flag,
             issue=issue,
         )
 
     message = exception.value.args[0]
 
     assert "DEPRECATION: Stop doing this!" in message
     assert "1.0" in message
+    assert str(feature_flag) not in message
     # Ensure non-None values are mentioned.
     for item in [replacement, issue]:
         if item is not None:
             assert str(item) in message
 
 
 @pytest.mark.usefixtures("patch_deprecation_check_version")
-def test_deprecated_message_reads_well():
+def test_deprecated_message_reads_well_past():
     with pytest.raises(PipDeprecationWarning) as exception:
         deprecated(
-            "Stop doing this!",
+            reason="Stop doing this!",
             gone_in="1.0",  # this matches the patched version.
             replacement="to be nicer",
-            issue="100000",  # I hope we never reach this number.
+            feature_flag="magic-8-ball",
+            issue="100000",
         )
 
     message = exception.value.args[0]
 
     assert message == (
         "DEPRECATION: Stop doing this! "
-        "This behavior change has been enforced since pip 1.0. "
+        "Since pip 1.0, this is no longer supported. "
+        "A possible replacement is to be nicer. "
+        "Discussion can be found at https://github.com/pypa/pip/issues/100000"
+    )
+
+
+@pytest.mark.usefixtures("patch_deprecation_check_version")
+def test_deprecated_message_reads_well_future():
+    with pytest.warns(PipDeprecationWarning) as record:
+        deprecated(
+            reason="Stop doing this!",
+            gone_in="2.0",  # this is greater than the patched version.
+            replacement="to be nicer",
+            feature_flag="crisis",
+            issue="100000",
+        )
+
+    assert len(record) == 1
+    message = record[0].message.args[0]
+
+    assert message == (
+        "DEPRECATION: Stop doing this! "
+        "pip 2.0 will enforce this behaviour change. "
         "A possible replacement is to be nicer. "
-        "Discussion can be found at "
-        "https://github.com/pypa/pip/issues/100000."
+        "You can use the flag --use-feature=crisis to test the upcoming behaviour. "
+        "Discussion can be found at https://github.com/pypa/pip/issues/100000"
     )