Make arguments to @deprecated/warn_deprecated keyword-only. #12440
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
(except for the deprecated-since version)
In 2.2 there were quite a few deprecation warnings of the form
```
warn_deprecated("2.2", "name-of-the-deprecated-API")
```
(e.g. passing 'box-forced' to set_adjustable, or the 'fig' kwarg to
get_subplot_params).
Such warnings would just display the name of the deprecated API when
triggered, without actually including a deprecation message or the
deprecated-since version. This is because the correct call would have
been
```
warn_deprecated("2.2", name="name-of-the-deprecated-API")
```
(leaving `message` -- the first arg -- to None, and getting an
autogenerated message).
To avoid this, make all args to `warn_deprecated` and `@deprecated`
keyword-only (except the deprecated-since version).
There is no deprecation period on the old signature of these deprecator
functions(!) because they are clearly intended for internal use, because
handling signature changes is a bit of a pain and because deprecations
on the deprecation machinery is a bit too meta.
9d51663 to
3f12eae
Compare
|
I'm confused, there don't seem to be any changes to |
anntzer
commented
Oct 8, 2018
| @@ -52,8 +52,8 @@ def _generate_deprecation_message( | |||
|
|
|||
|
|
|||
| def warn_deprecated( | |||
| since, message='', name='', alternative='', pending=False, | |||
| obj_type='attribute', addendum='', *, removal=''): | |||
| since, *, message='', name='', alternative='', pending=False, | |||
There was a problem hiding this comment.
that's the relevant change
timhoffm
approved these changes
Oct 8, 2018
There was a problem hiding this comment.
To me, even since could be keyword-only. I've been confused in the beginning if it's the version this got deprecated in or if it's the version this will be removed in. Getting used to it now as I encounter it more and more, but it's not self-explanatory for new readers of the code.
|
I think that's too wordy to be worth it (that's literally all calls to deprecated/warn_deprecated), but I don't care that much either way. |
|
@dstansby (pinging you as you already reviewed above) Are you OK with the changes? |
dstansby
approved these changes
Nov 1, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(except for the deprecated-since version)
In 2.2 there were quite a few deprecation warnings of the form
(e.g. passing 'box-forced' to set_adjustable, or the 'fig' kwarg to
get_subplot_params).
Such warnings would just display the name of the deprecated API when
triggered, without actually including a deprecation message or the
deprecated-since version. This is because the correct call would have
been
(leaving
message-- the first arg -- to None, and getting anautogenerated message).
To avoid this, make all args to
warn_deprecatedand@deprecatedkeyword-only (except the deprecated-since version).
There is no deprecation period on the old signature of these deprecator
functions(!) because they are clearly intended for internal use, because
handling signature changes is a bit of a pain and because deprecations
on the deprecation machinery is a bit too meta.
PR Summary
PR Checklist