-
Notifications
You must be signed in to change notification settings - Fork 56
refactor: Update deprecate arguments. #4553
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
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.
Pull Request Overview
This PR introduces a new deprecate_arguments
decorator to centralize and simplify argument deprecation handling. The decorator supports grouped argument mappings, custom conversion logic, and consistent warning messages across the API.
- Adds a flexible decorator that handles multiple deprecated arguments with automatic replacement
- Supports both default 1-to-1 mapping and custom conversion functions for complex argument transformations
- Provides consistent deprecation warnings with configurable warning classes
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Hi, I have created this PR with the new signature. Please provide me your comments by referring to the example usages, based on your feedback will do improvements. I have added some sample implementations in launcher, field_data and file_session code for reference. Please note: method deprecator is yet to be implemented. Thank you. |
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
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
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
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
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
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/ansys/fluent/core/utils/deprecate_new.py:1
- The
_show_gui_to_ui_mode
function is not defined in this file and is being called without being imported. This will cause a NameError at runtime.
"""Deprecate Arguments."""
src/ansys/fluent/core/utils/deprecate_new.py:1
- The
_version_to_dimension
function is not defined in this file and is being called without being imported. This will cause a NameError at runtime.
"""Deprecate Arguments."""
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
@prmukherj Will there be a follow-up stage to completely supersede the old version?
yes, I have created this PR just to get feedback on the design from everyone. Now I'll mark this as draft and make the shift as part of this PR only. Thank you for your feedback @seanpearsonuk |
OK, feel free to merge in stages too. |
>>> @deprecate_arguments(
>>> old_args=["a", "b"],
>>> new_args="d",
>>> version="3.0.0",
>>> converter=my_custom_converter,
>>> )
>>> @deprecate_arguments(
>>> old_args="c",
>>> new_args="e",
>>> version="3.0.0",
>>> converter=my_custom_converter,
>>> )
>>> def example_func(**kwargs):
>>> print(kwargs) |
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
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@prmukherj The latest iteration looks a lot simpler and cleaner. |
if isinstance(new_args, str): | ||
new_args = [new_args] | ||
|
||
# Validation |
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.
# Validation |
# Default converter | ||
def _default_converter( |
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.
# Default converter | |
def _default_converter( | |
def _default_converter( |
return decorator | ||
converter = converter or _default_converter | ||
|
||
# Build reason message for @deprecated |
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.
# Build reason message for @deprecated |
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.
Looks like a significant improvement to me, thanks for asking for review. Note the tests are failing, I didn't investigate if they are related to these changes or not
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
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Context
In the existing codebase, argument deprecations were handled inconsistently and often required writing repetitive, manual logic to warn users and remap deprecated arguments to their newer equivalents.
There was no unified or flexible way to deprecate multiple arguments at once, especially when multiple old arguments needed to map to one or more new arguments.
Additionally, existing deprecation decorators typically required developers to hardcode version strings and custom messages, leading to maintenance overhead and a higher chance of inconsistencies.
Change Summary
A new decorator, deprecate_arguments, is introduced with the following signature:
A new decorator for deprecating function is also introduced.
Rationale
This decorator centralizes and simplifies how argument deprecations are declared and processed.
It reduces boilerplate, improves readability, and enforces consistency across the API.
By allowing grouped mappings, the design accommodates complex transitions (such as merging or splitting arguments).
Passing the full old_arg_list and new_arg_list to the converter provides more flexibility for complex transformation logic while keeping the default case straightforward.
Impact
Simplifies the process of marking arguments as deprecated.
Reduces risk of missing or inconsistent deprecation messages.
Provides an easy path for custom conversion logic between old and new APIs.
Example usage