-
-
Notifications
You must be signed in to change notification settings - Fork 194
MNT: deprecated decorator #830
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
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very interesting implementation.
I know that Python 3.13 added a similar decorator. I don't think it has the same custom functionalities (such as providing an alternative). Nonetheless, it is worth the read.
Since this Python is far from being RocketPy minimum required version, even if we were to use it, a custom version would be needed to support other Python versions (similarly to what happed with cached_property
a few years ago). Therefore, I agree with this PR implementation.
Optionally, we could discuss the possibility of changing the parameters so that it is compatible with the Python standard one when/if we were to change to it in the future. Another idea for discussion is having the Python default one if the user has a >= 3.13
Python version.
8e2824a
to
cb62905
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
A pull request introducing a reusable deprecated
decorator and replacing inline deprecation warnings with that decorator for legacy methods.
- Added a
deprecated
decorator inrocketpy/tools.py
to standardize deprecation notices. - Removed manual
warnings.warn(...)
calls fromRocket.add_fins
andFunction.plot1D/plot2D
, replacing them with the new decorator. - Updated imports in affected modules to include the
deprecated
decorator.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
rocketpy/tools.py | Added warnings import and defined the deprecated decorator. |
rocketpy/rocket/rocket.py | Imported deprecated ; applied decorator to add_fins and removed manual warning. |
rocketpy/mathutils/function.py | Imported deprecated ; applied decorator to plot1D /plot2D and removed manual warnings. |
Comments suppressed due to low confidence (2)
rocketpy/tools.py:32
- Consider adding unit tests for the new
deprecated
decorator to verify that it emits the correct warnings and preserves function metadata.
def deprecated(reason=None, version=None, alternative=None):
rocketpy/tools.py:68
- The decorator uses
functools.wraps
butfunctools
is not imported. Addimport functools
at the top of the file.
@functools.wraps(func)
I agree - it's great that Python 3.13 introduces a standard deprecation decorator, but as you mentioned, our minimum supported version is still far from that, so a custom solution makes sense for now. I also appreciate your point about aligning our decorator’s parameters with the standard one for easier transition in the future; that sounds like a good idea and would make any future migration smoother. Regarding feature-detection for Python ≥3.13, we could look into conditionally using the standard decorator when available, and fall back to our custom one otherwise. However, I believe the benefits from that would not be significant. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #830 +/- ##
===========================================
+ Coverage 79.11% 80.02% +0.90%
===========================================
Files 96 98 +2
Lines 11575 12003 +428
===========================================
+ Hits 9158 9605 +447
+ Misses 2417 2398 -19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Solves #669