Repository: pypa/pip
Pull request #10229 (2021-07-28 23:14:53)

Give some TLC to the `deprecated` helper
Inspired by the review of #10167 

maresb: Very nice cleanup and tests. Just a few comments...

1. This is conflicting with my original #10129. There I use the `feature_flag=` kwarg and change the wording slightly. I no longer feel strongly about the wording, but let's at least use `feature_flag=`.

2. I switched to named variables on @uranusjr's [request](https://github.com/pypa/pip/pull/10167#discussion_r671798540) but you are switching back. Personally, I regret not noticing that `python_requires=">=3.6"` and wish that I had used f-strings. I don't see any reason to be using the `format()` function here. To be explicit, why don't we just do
    ```python
    sentences = [
        ....
        f"a possible replacement is {replacement}.",
        ....
    ```

3. This regresses #10223, preventing you from potentially leaving behind helpful error messages after functionality is removed. (But if you're sure you'll never do this in the future then it doesn't matter.)

It just seems like this PR undoes many of my "improvements"... but that's fine, I have no more contributions to make, so just do whatever you see fit. Thanks!
uranusjr: Agreed. If we are making these string literals local, we should use f-strings. But I think making these global variables (plus named fields) would make lives *much* easier for downstream distributors.
pradyunsg: 
> It just seems like this PR undoes many of my "improvements"... but that's fine, I have no more contributions to make, so just do whatever you see fit. Thanks!

Oh no. :(

I apologise for making you feel this way -- that's not the intent here. I'll note my line of reasoning that led to me filing this PR at the end; hopefully that helps clarify my intent here.

> 2\. named variables on @uranusjr's [request](https://github.com/pypa/pip/pull/10167#discussion_r671798540) but you are switching back

Ah, I missed that it was an explicit request. I would've pushed back then, if I had noticed that originally. I don't like the layer of indirection and the reasoning presented so far in favour of that is something I disagree with.

> 3\. This regresses https://github.com/pypa/pip/issues/10223

Kinda. One of the conditions added there is still here with less boilerplate if/else statements around it IMO.

https://github.com/pypa/pip/commit/575ff54062087e9bc8bc9624d52a8540c1c89755#diff-66013b676df28e23258eaaf2062a46e54631e57059adafa12b6c46dd2bc06e3cR89-R92

I did miss that there was another conditional added for the `feature_flag`, which I'll add back now. That I missed it while trying to reimplement this is (IMO) a symptom of the fact that the additional boilerplate if/else statements make it difficult to follow this code.

> 1. This is conflicting with my original #10129

There are no intended behaviour changes -- the behaviour of this PR should match what #10129 expects. Any mismatch in behaviour is a bug in this PR.

This is conflicting with that PR though, since there's a merge conflict here -- this PR enforces keyword-only arguments. You're updating the message to to add the keyword + rephrase, I'm updating the message to add the keyword + reindent -- git won't like that there's two changes for the same lines. I'm happy to handle the conflict here by merging your PR first, so that you don't have to. :)

---

> If we are making these string literals local, we should use f-strings.

We can, that also means we'd need to manually make sure that the variable we're formatting with matches the one we checked for `None`.

```py
parts = [
   (one, "some {} value"),
   (two, "some other {} value"),
]

message = "\n".join([s.format(value) for value, s in parts if value is not None])
```

```py
parts = [
   (one, f"some {one} value"),
   (two, f"some other {two} value"),
]

message = " ".join([s for value, s in parts if value is not None])
```

I don't think adding f-strings does much here to improve readability and adds a potential spot for making a mistake due to the fact that there's manual repetition. The message + conditional variable don't actually fit into the same line in most cases, so there'd also be visual separation that the examples above don't showcase.

The same is not true, however, if we don't switch to using a list for checking values:

```
formatted_one_message = None if one is None else "some {one} value.".format(one=one)
formatted_two_message = None if two is None else "some other {two} value.".format(two=two)

message = " ".join([formatted_one_message, formatted_two_message])
```

```
formatted_one_message = None if one is None else f"some {one} value."
formatted_two_message = None if two is None else f"some other {two} value."

message = " ".join(filter(None, [formatted_one_message, formatted_two_message]))
```

I still prefer the first block though, since we're adding additional logic for the `formatted_*_message` conditionals that's obscuring the "meat" of the logic in this helper. :)

> But I think making these global variables (plus named fields) would make lives _much_ easier for downstream distributors.

I don't think we want these to be customisable by downstream distributors. And even if we do, the mechanism they'd have to use is a patch; which can equally modify this code as well.

I don't see how adding a layer of indirection for them is worth making it more difficult to understand what the message will be by looking at the function's implementation by breaking the locality of it.

---

Now for the context/perspective I promised earlier.

I noticed that we can now use keyword-only arguments, and figured that it would be a good idea to get that done for this function. It wasn't a blocking concern for your PRs, so I figured I'll do it myself since I had a bit of time at hand. I then noticed that we were missing tests for the newly added feature_flag argument, so I added those.

Then, I noticed the original approach of `(value, format_string)` could still work since the code that I was looking at now was a lot of boilerplate `s.format(...) if s is not None else None` -- so I tried to adapt things and got something that passed the tests; so I included that as well! Now, as noted above, I don't agree with the reasoning behind having a layer of indirection here, and well, given that I didn't know this was something that someone wanted to do explicitly, hopefully you can see why I did all this.


pradyunsg: > I did miss that there was another conditional added for the `feature_flag`, which I'll add back now.

Done!
maresb: > > It just seems like this PR undoes many of my "improvements"... but that's fine, I have no more contributions to make, so just do whatever you see fit. Thanks!
>
> Oh no. :(
>
> I apologise for making you feel this way -- that's not the intent here. I'll note my line of reasoning that led to me filing this PR at the end; hopefully that helps clarify my intent here.

Ah, I think my comment came off wrong. I don't have any hurt feelings, rather I feel good about our interactions. Even though we seem to disagree quite often, I appreciate and learn from your perspective. I won't be contributing more to pip, simply because I have way too much other stuff to do, not because I'm disappointed. Besides, this is critical Python infrastructure where I'm a fleeting contributor, and you must defend the integrity and style of the codebase.

---

Thanks for adding back the conditional with `feature_flag`! (Sorry, I should have been more explicit in my comment that the problem was with `feature_flag`.) Now we're good for #10223.

I very much agree with enforcing kwargs instead of positional args. I think that's a huge improvement for readability.

Regarding the formatting, it seems that we're only debating the style, so no big deal. As for my reasoning with that, my personal style is to avoid compound statements. I have trouble reading compound statements in other people's code, so I'd rather make my code longer and introduce variables with self-documenting names. I find it curious that you find that more difficult to read. :smile: Maybe my personal style needs improvement.

My suggestion for the f-strings would be more like:

```python
parts = [
   f"some {one} value" if one else None,
   (f"some other {two} value" if use_another else f"another {one} value") if two else None,
]

message = " ".join([s for s in parts if s is not None])
```

But I find the second entry of `parts` to be very confusing to read. That's why I'd rather do something like

```python

second_formatted_str = f"some other {two} value" if use_another else f"another {one} value"
second_formatted_str = second_formatted_str if two else None

parts = [
   f"some {one} value" if one else None,
   second_formatted_str,
]
```

and perhaps for uniformity treat the first string similarly.

Rather than seeing it as boilerplate, I consider this latter option to be more robust: it's easy to extend things in the future with more complex logic by simply adding more lines. (With your solution, the tendency would be that all extensions of the logic should occur within the single line as a compound statement.)

But this is all just style. The functionality should now be identical in any case. But it's interesting for me to get your feedback regarding my style! :smile:
pfmoore: FWIW, I find it hard to follow the coding style discussion with abstract examples like "if one" and "if two", but with that caveat, I do find @maresb's version with intermediate variables easier to read than the list with nested if-expressions.

Also, complex expressions like the current code in this PR are precisely the sort of thing black enjoys mangling in ways that I hate, so I'm in favour of the multi-statement form so we avoid that issue, as well 🙂 
pradyunsg: I personally find the form that's currently in `main` *much* more difficult to follow. The exact expressions are spread across the file and there's an additional layer of indirection that I really don't find particularly interesting to follow.

So let me turn this discussion around a bit -- does anyone have blocking concerns with the style I'm proposing in this PR? If it's not something you can follow and/or work with, please state so. Otherwise, I'd like to get the proposed style merged in, since it's much easier for me to follow and will make it easier for me to write the deprecation messages and to review PRs with deprecation messages when the author doesn't post sample output.

> ```python
>    (two, "some other {} value" if use_another else "another {} value"),
> ```

> ```python
>    (two, f"some other {two} value" if use_another else f"another {one} value"),
> ```

That nobody noticed or at least bothered to note that the behaviour between these two examples is different, is reason enough for me to feel that avoiding f-strings here is A-OK.

> complex expressions like the current code in this PR are precisely the sort of thing black enjoys mangling in ways that I hate, so I'm in favour of the multi-statement form so we avoid that issue,

FWIW, I did realise that I was overcomplicating things. In other words, the way that black formatted the code did result in me writing easier to read code. :)

pfmoore: > So let me turn this discussion around a bit -- does anyone have blocking concerns with the style I'm proposing in this PR?

I'm not 100% sure what you're referring to here, but assuming you're referring to the code style used in the implementation of `deprecated` (as opposed to, say, a particular style *callers* have to follow), I don't really care. I'm unlikely to ever bother modifying the implementation of `deprecated` so IMO it's people who might who have the final say here.
pradyunsg: > assuming you're referring to the code style used in the implementation of `deprecated`

Yes.
maresb: @pradyunsg I was thinking of doing a PR on your PR, but I'm still traveling, and I wouldn't be able to get to it for quite some time. Since you are maintaining the code, I think it makes the most sense to do whatever makes most sense to you. So I have no objections.

Patch:
diff --git a/src/pip/_internal/locations/__init__.py b/src/pip/_internal/locations/__init__.py
index 2c2fd860a..d74857b2a 100644
--- a/src/pip/_internal/locations/__init__.py
+++ b/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,
         )
diff --git a/src/pip/_internal/utils/deprecation.py b/src/pip/_internal/utils/deprecation.py
index a163395c4..72bd6f25a 100644
--- a/src/pip/_internal/utils/deprecation.py
+++ b/src/pip/_internal/utils/deprecation.py
@@ -8,18 +8,9 @@ from typing import Any, Optional, TextIO, Type, Union
 
 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)


Test patch:
diff --git a/tests/functional/test_warning.py b/tests/functional/test_warning.py
index 9272abb92..9f4534222 100644
--- a/tests/functional/test_warning.py
+++ b/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
 
diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py
index 4e5bba2e1..c9228bc02 100644
--- a/tests/unit/test_utils.py
+++ b/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,7 +897,7 @@ 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
 
@@ -901,12 +905,14 @@ def test_deprecated_message_contains_information(gone_in, replacement, issue):
 @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,
         )
 
@@ -914,6 +920,7 @@ def test_deprecated_raises_error_if_too_old(replacement, issue):
 
     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:
@@ -921,23 +928,46 @@ def test_deprecated_raises_error_if_too_old(replacement, issue):
 
 
 @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"
     )