Include version in deprecation warnings #6218
Conversation
pradyunsg
added
type: enhancement
pradyunsg
added
the
skip news
|
Adding tests for the case when exceptions are raised. |
pradyunsg
force-pushed
the
fix/include-version-in-deprecation-warnings
branch
from
9263bb4
to
eb8b275
Compare
|
@cjerdonek Could you take a look at this PR? |
| @@ -83,8 +83,10 @@ def deprecated(reason, replacement, gone_in, issue=None): | |||
| if issue is not None: | |||
| url = "https:/pypa/pip/issues/" + str(issue) | |||
| message += " You can find discussion regarding this at {}.".format(url) | |||
| if gone_in is not None: | |||
| message += " pip {} will remove this functionality.".format(gone_in) | |||
There was a problem hiding this comment.
I think the most important information should be listed first. So that would be (1) the version in which it was / will be removed, (2) then the replacement, and (3) ending with where to go for more info.
| message += " pip {} will remove this functionality.".format(gone_in) | ||
| # Raise as an error if it has to be removed. | ||
| if parse(current_version) >= parse(gone_in): | ||
| raise PipDeprecationWarning(message) |
There was a problem hiding this comment.
In this case, I would make the message say, "was removed in" instead of "will be."
|
|
||
| assert "DEPRECATION: Stop doing this!" in message | ||
| # Ensure non-None values are mentioned. | ||
| for item in [gone_in, replacement, issue]: |
There was a problem hiding this comment.
With this combinatorial way of testing, it's hard to visualize what the full message looks like, so, in addition to the test testing the various combinations, I would add one separate test with all the information present testing an exact match on the string. That gives maintainers a way to check the message as a whole. I would do one test like this for the exception case and one test for the "will be" case.
|
This isn't critical path for 19.0.2. |
|
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
|
Closing this and creating a new PR because I'm bored. |
The warning getting raised, would only be seen by the maintainers, right? I don't think we need to do this. |
More because it was easier to copy-paste than fiddle around with git-rebase for such a small PR. I was too lazy to write that. |
lock
bot
added
the
auto-locked
Closes #6216