Deprecation decorators for functions #4757
Conversation
RohitP2005
requested review from
martinjrobins,
Saransh-cpp,
agriyakhetarpal,
kratman,
arjxn-py,
a team,
rtimms,
valentinsulzer,
TomTranter,
MarcBerliner,
brosaplanella and
DrSOKane
as code owners
|
Previous Discussions : #4745 (comment) |
There was a problem hiding this comment.
I think one of the main reasons for doing this is to track the date of the deprecation warnings. This does not seem to be in these changes.
Realistically the warnings work as-is, but we want to automate the removal of them. Right now I use git-blame to remove old warnings every 6 months or so. The extra decorators and such are just additional overhead unless we use it for something better than the current warnings
|
Original issue request: "Replace existing deprecation warnings/errors with deprecation package so tests fail after a few releases." |
|
Okay , I misunderstood that the deprecation package was not required and we needed a pythonic way. I will just go ahead with the deprecation package with my next commit Apologies for the confusion |
RohitP2005
force-pushed
the
issue#2028
branch
from
5024850 to
ff6a817
Compare
|
@kratman, I've implemented the deprecation package for four of the existing deprecation warnings. Could you please guide me on any potential improvements? Regarding renamed and deprecated arguments, it seems the deprecation package doesn't natively handle these cases. Do you have any suggestions on how to approach this? I considered writing custom decorators to manage them. Does that sound like a good plan? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4757 +/- ##
========================================
Coverage 98.66% 98.66%
========================================
Files 303 303
Lines 23227 23233 +6
========================================
+ Hits 22916 22922 +6
Misses 311 311 ☔ View full report in Codecov by Sentry. |
|
Does the package you use allow for tests to fail when the deprecation is old? |
| msg = "pybamm.simulation.set_up_and_parameterise_experiment is deprecated and not meant to be accessed by users." | ||
| warnings.warn(msg, DeprecationWarning, stacklevel=2) |
There was a problem hiding this comment.
Do we need to use warnings.warn here since we have already used the deprecated decorator above? The msg is repetitive, as already defined above.
There was a problem hiding this comment.
Yeah, I planned on removing them on the next commit . once my Eric reviewed it and was ok with the implementation.
| msg = ( | ||
| "pybamm.set_parameters is deprecated and not meant to be accessed by users." |
No, the deprecation package does not natively support automatically failing tests when a deprecation is old |
Description
This pull request has implemented deprecation decorators as wrapper for a function's arguement and the fucntion itself and has added
-Wflag to ensure the deprecation warnings are treated as errorFixes #2028
Type of change
Key checklist:
$ pre-commit run --all-files(or$ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally)$ python -m pytest -W error(or$ nox -s tests) (ensure warnings are treated as errors to catch deprecation notices)$ python -m pytest --doctest-plus src(or$ nox -s doctests)You can run integration tests, unit tests, and doctests together at once using
$ nox -s quick.Further checks: