Give some TLC to the deprecated helper

[pradyunsg]

Inspired by the review of #10167

[maresb]

Very nice cleanup and tests. Just a few comments...

1. This is conflicting with my original Improve the phrasing of in-tree build deprecation warning #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 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

   sentences = [
       ....
       f"a possible replacement is {replacement}.",
       ....

3. This regresses deprecated() function refers to past changes as if they were upcoming. #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 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 #10223

Kinda. No. One of the conditions added there is still here with less boilerplate if/else statements around it IMO.

575ff54#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. 😅

Update: The missed conditional has been added back now, with tests.

1. This is conflicting with my original Improve the phrasing of in-tree build deprecation warning #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. :)

---

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.

parts = [
   (one, "some {} value"),
   (two, "some other {} value" if use_another else "another {} value"),
]

message = "\n".join([s.format(value) for value, s in parts if value is not None])

parts = [
   (one, f"some {one} value"),
   (two, f"some other {two} value" if use_another else f"another {one} 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 add a potential spot for making a mistake due to the fact that there's manual repetition.

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)
two_message = "some other {two} value" if use_another else "another {two} value"
formatted_two_message = None if two is None else two_message.format(two=two)

message = " ".join([formatted_one_message, formatted_two_message])

formatted_one_message = None if one is None else f"some {one} value."
two_message = f"some other {two} value" if use_another else f"another {two} value"
formatted_two_message = None if two is None else two_message

message = " ".join(filter(None, [formatted_one_message, formatted_two_message]))

I still prefer the first two blocks though (1,2,4,3 is my ordering; if you're wondering), since we're adding additional logic for the formatted_*_message conditionals that's obscuring the "meat" of the logic in this helper. :)

ALSO, 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.

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. 😄 Maybe my personal style needs improvement.

My suggestion for the f-strings would be more like:

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

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! 😄

[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.

   (two, "some other {} value" if use_another else "another {} value"),

   (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. If someone really want to switch, I won't block a PR doing so in a follow up.

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 hate that too. That said, what black does to these expressions serves as a hint to me that I might be overcomplicating things, which was the case here. 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.