Skip to content

Add decorator for deprecation tests#105

Open
A-CGray wants to merge 4 commits into
mainfrom
feature-DepTestDecorator
Open

Add decorator for deprecation tests#105
A-CGray wants to merge 4 commits into
mainfrom
feature-DepTestDecorator

Conversation

@A-CGray

@A-CGray A-CGray commented Jun 1, 2026

Copy link
Copy Markdown
Member

Purpose

When we make a backward incompatible change in our code (e.g changing the name of a function or argument), it is good practice to implement a "shim" so that code following the old API still works, then remove that shim in a later release (usually 2 minor versions later). However, we don't currently do anything to actually remind us to remove these shims.

This PR adds a decorator that both handles the deprecation warning, and raises an error if the shim is still present once the relevant package reaches the version the shim is supposed to be removed in.

e.g:

@expire_deprecation(package_name="pygeo", removal_version="1.20", new_name="getDesignVars")
def getValues(self):
    return self.getDesignVars()

When called in version < 1.20:

/home/ali/repos/pygeo/paper/Global-FFD-DV-Demo.py:46: DeprecationWarning: getValues() is deprecated and will be removed in pygeo v1.20. Use getDesignVars instead.
  DVGeo.getValues()

When called in version >= 1.20:

Traceback (most recent call last):
  File "/home/ali/repos/pygeo/paper/Global-FFD-DV-Demo.py", line 8, in <module>
    from pygeo import DVGeometry
  File "/home/ali/repos/pygeo/pygeo/__init__.py", line 7, in <module>
    from .parameterization import DVGeometry
  File "/home/ali/repos/pygeo/pygeo/parameterization/__init__.py", line 1, in <module>
    from .DVGeo import DVGeometry
  File "/home/ali/repos/pygeo/pygeo/parameterization/DVGeo.py", line 18, in <module>
    from .BaseDVGeo import BaseDVGeometry
  File "/home/ali/repos/pygeo/pygeo/parameterization/BaseDVGeo.py", line 15, in <module>
    class BaseDVGeometry(ABC):
  File "/home/ali/repos/pygeo/pygeo/parameterization/BaseDVGeo.py", line 69, in BaseDVGeometry
    @expire_deprecation(package_name="pygeo", removal_version="1.20", new_name="getDesignVars")
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ali/repos/baseclasses/baseclasses/testing/decorators.py", line 92, in decorator
    raise AssertionError(
AssertionError: BaseDVGeometry.getValues: deprecated API should have been removed in pygeo v1.20 (current: v1.20.0). Please delete this shim.

Expected time until merged

Type of change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (please describe)

Testing

Checklist

  • I have run ruff check and ruff format to make sure the Python code adheres to PEP-8 and is consistently formatted
  • I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

@A-CGray A-CGray requested a review from a team as a code owner June 1, 2026 17:59
@A-CGray A-CGray requested a review from sanjan98 June 1, 2026 17:59
@A-CGray A-CGray mentioned this pull request Jun 1, 2026
13 tasks
@A-CGray A-CGray requested a review from ewu63 June 1, 2026 18:07
@ewu63

ewu63 commented Jun 1, 2026

Copy link
Copy Markdown
Collaborator

I'm proposing a slightly different approach (which works in my head but did not test this out). Because the decorator runs at import time, we may be able to do two improvements

  1. Raise the error in the decorator and skip any new tests. As long as we are importing this module somewhere in the tests, that import should fail and a bunch of tests will fail, without having to write dedicated deprecation tests.
  2. Apply the decorator on the shim, and we can auto-generate the warning message within the decorator too, so that the shim becomes super lightweight.

MVP:

def expire_deprecation(package_name: str, removal_version: str, new_name: str = None):
    removal = Version(removal_version)
    current = Version(version(package_name))

    def decorator(func):
        if current >= removal:
            raise AssertionError(
                f"{func.__qualname__}: deprecated API should have been "
                f"removed in {package_name} v{removal} "
                f"(current: v{current}). Please delete this shim."
            )

        msg = (
            f"{func.__name__}() is deprecated and will be removed in "
            f"{package_name} v{removal}. Use {new_name}() instead."
            if new_name else
            f"{func.__name__}() is deprecated and will be removed in "
            f"{package_name} v{removal}."
        )

        @functools.wraps(func)
        def wrapper(self, *args, **kwargs):
            warnings.warn(msg, DeprecationWarning, stacklevel=2)
            return func(self, *args, **kwargs)

        return wrapper

    return decorator

And use it as

@expire_deprecation("pygeo", "1.20", new_name="getDesignVars")
def getValues(self):
    return self.getDesignVars()

@A-CGray

A-CGray commented Jun 2, 2026

Copy link
Copy Markdown
Member Author

I'm proposing a slightly different approach (which works in my head but did not test this out). Because the decorator runs at import time, we may be able to do two improvements

1. Raise the error in the decorator and skip any new tests. As long as we are importing this module somewhere in the tests, that import should fail and a bunch of tests will fail, without having to write dedicated deprecation tests.

2. Apply the decorator on the shim, and we can auto-generate the warning message within the decorator too, so that the shim becomes super lightweight.

MVP:

def expire_deprecation(package_name: str, removal_version: str, new_name: str = None):
    removal = Version(removal_version)
    current = Version(version(package_name))

    def decorator(func):
        if current >= removal:
            raise AssertionError(
                f"{func.__qualname__}: deprecated API should have been "
                f"removed in {package_name} v{removal} "
                f"(current: v{current}). Please delete this shim."
            )

        msg = (
            f"{func.__name__}() is deprecated and will be removed in "
            f"{package_name} v{removal}. Use {new_name}() instead."
            if new_name else
            f"{func.__name__}() is deprecated and will be removed in "
            f"{package_name} v{removal}."
        )

        @functools.wraps(func)
        def wrapper(self, *args, **kwargs):
            warnings.warn(msg, DeprecationWarning, stacklevel=2)
            return func(self, *args, **kwargs)

        return wrapper

    return decorator

And use it as

@expire_deprecation("pygeo", "1.20", new_name="getDesignVars")
def getValues(self):
    return self.getDesignVars()

I like this functionality, except that this only works for this specific kind of deprecation (renaming a function). If we want to go with this approach and have different decorators for different types of deprecation then we could also just go whole hog and use a library like pyDeprecate.

@ewu63

ewu63 commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator

I like this functionality, except that this only works for this specific kind of deprecation (renaming a function). If we want to go with this approach and have different decorators for different types of deprecation then we could also just go whole hog and use a library like pyDeprecate.

I think it's an improvement over the current PR and should handle rename/removal which are the most common scenarios. Anything more than this then probably will require a more complex solution -- generally I would prefer not adding another dependency for something like this. I just find it clunky to have to add a new test for each deprecation.

@A-CGray

A-CGray commented Jun 11, 2026

Copy link
Copy Markdown
Member Author

Implemented your suggestions @ewu63

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants