-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce a helper function for deprecation #5542
Introduce a helper function for deprecation #5542
Conversation
One thing I don't like about this, is it removes the note about when a particular deprecation can be removed. I think that's important because otherwise we'll end up with stuff that has been marked deprecated for ages that we've just forgotten exists. Now that we have date based releases, it should be pretty easy to handle this though (easier than the old way), just set the version it's going to be removed in, and once we're at that version, raise exceptions instead of warnings so that our tests break and force us to remove them. |
I wasn't sure how the logic should behave if we skipped a release for the year. Say something had to go away in 19.3 but we skipped 19.2 in the first release month for it so, it came 3 months later. How should the deprecation warnings/errors behave (19.3 could also be 20.1 or something instead)? |
I wouldn't skip version numbers, if 19.2 is the next version, it should be the next version, unless we roll over to the next year. However, maybe the better thing to do is instead of making the deprecation policy release based, we make it time based as well. We can say that our policy is items will emit deprecation warnings for at least 6 months (so that's 2 releases worth of deprecation warnings). You can then just do a
Or another scenario:
|
#5572 has some recommended language to adjust the deprecation policy by. |
I don't think there's much to gain from preserving the current logger.warning -> logger.error progression. Does anyone mind if I remove that? If so, please let me know what logic you'd want to apply for deciding between which method to use. |
I'm fine with getting rid of it. |
One thing I'd like to add to this, is a little bit of documentation in the deprecation policy area about how to use this function, and what values should be passed into this. |
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 |
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 |
It doesn't represent warnings.showwarning's current state so, the name shouldn't suggest that.
I noticed @BrownTruck -- was running tests locally after a rebase to make sure I did it correctly. (why am I talking to a bot) |
2df090a
to
74dd124
Compare
Also: - Remove conditional warning/error level logging - Remove now-obsolete class - Update call sites as per new signature
74dd124
to
b6dae7a
Compare
@dstufft Anything else? |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Built on top of #5523
Replaces the current deprecation setup with one which has a single function -- the new
deprecated
function also includes 3 optional parameters:Providing these should make it easier for users to reach places with more context and also make it more explicit that users need to be provided advice about how to deal with the deprecation.