laurent-laporte-pro / deprecated

Python @deprecated decorator to deprecate old python classes, functions or methods.
MIT License
307 stars 34 forks source link

fix: add `extra_stacklevel` argument to better control deprecated function call references #69

Closed coroa closed 1 year ago

coroa commented 1 year ago

Fixes #68 .

Adds an argument extra_stacklevel to the classic and sphinx deprecated functions (and adapters) to modify the stacklevel which the warnings refer to.

Documentation of the patch is still weak and an explicit functional test needs to be added.

tantale commented 1 year ago

Suggested unit test:

import inspect
import warnings

import deprecated.classic

def test_default_stacklevel():
    """
    The objective of this unit test is to ensure that the triggered warning message,
    when invoking the 'use_foo' function, correctly indicates the line where the
    deprecated 'foo' function is called.
    """

    @deprecated.classic.deprecated
    def foo():
        pass

    def use_foo():
        foo()

    with warnings.catch_warnings(record=True) as warns:
        warnings.simplefilter("always")
        use_foo()

    # Check that the warning path matches the module path
    warn = warns[0]
    assert warn.filename == __file__

    # Check that the line number points to the first line inside 'use_foo'
    use_foo_lineno = inspect.getsourcelines(use_foo)[1]
    assert warn.lineno == use_foo_lineno + 1

def test_extra_stacklevel():
    """
    The unit test utilizes an 'extra_stacklevel' of 1 to ensure that the warning message
    accurately identifies the caller of the deprecated function. It verifies that when
    the 'use_foo' function is called, the warning message correctly indicates the line
    where the call to 'use_foo' is made.
    """

    @deprecated.classic.deprecated(extra_stacklevel=1)
    def foo():
        pass

    def use_foo():
        foo()

    def demo():
        use_foo()

    with warnings.catch_warnings(record=True) as warns:
        warnings.simplefilter("always")
        demo()

    # Check that the warning path matches the module path
    warn = warns[0]
    assert warn.filename == __file__

    # Check that the line number points to the first line inside 'demo'
    demo_lineno = inspect.getsourcelines(demo)[1]
    assert warn.lineno == demo_lineno + 1
tantale commented 1 year ago

Suggested documentation

In docs/source/tutorial.rst:

Modifying the deprecated code reference
---------------------------------------

By default, when a deprecated function or class is called, the warning message indicates the location of the caller.

The ``extra_stacklevel`` parameter allows customizing the stack level reference in the deprecation warning message.

This parameter is particularly useful in scenarios where you have a factory or utility function that creates deprecated
objects or performs deprecated operations. By specifying an ``extra_stacklevel`` value, you can control the stack level
at which the deprecation warning is emitted, making it appear as if the calling function is the deprecated one,
rather than the actual deprecated entity.

For example, if you have a factory function ``create_object()`` that creates deprecated objects, you can use
the ``extra_stacklevel`` parameter to emit the deprecation warning at the calling location. This provides clearer and
more actionable deprecation messages, allowing developers to identify and update the code that invokes the deprecated
functionality.

For instance:

.. literalinclude:: tutorial/warning_ctrl/extra_stacklevel_demo.py

Please note that the ``extra_stacklevel`` value should be an integer indicating the number of stack levels to skip
when emitting the deprecation warning.

And a new demo in docs/source/tutorial/warning_ctrl/extra_stacklevel_demo.py:

import warnings

from deprecated import deprecated

@deprecated(version='1.0', extra_stacklevel=1)
class MyObject(object):
    def __init__(self, name):
        self.name = name

    def __str__(self):
        return "object: {name}".format(name=self.name)

def create_object(name):
    return MyObject(name)

if __name__ == '__main__':
    warnings.filterwarnings("default", category=DeprecationWarning)
    # warn here:
    print(create_object("orange"))
    # and also here:
    print(create_object("banane"))
coroa commented 1 year ago

Oh, we worked in parallel on that. Sorry, should have clarified i'll take care of it. Let me incorporate your suggestions. To understand my unit test there is a lot of prior knowledge involved.

Also I added an optional refactoring commit, which i think disentangles code responsibilities. I am happy to move that into a second PR, if that is preferred.

coroa commented 1 year ago

I think you also would have been allowed to directly push to this branch.

tantale commented 1 year ago

Oh, we worked in parallel on that. Sorry, should have clarified i'll take care of it. Let me incorporate your suggestions. To understand my unit test there is a lot of prior knowledge involved.

Also I added an optional refactoring commit, which i think disentangles code responsibilities. I am happy to move that into a second PR, if that is preferred.

Creating a new PR is not necessary.

tantale commented 1 year ago

Ok, I've just seen a regression regarding the Python 2.7 support. I fixed it on the master branch.

So, your code is correct. I have done some minor changes. I will rebase your branch on the master branch and then I will force push my changes to your branch so that you can see the changes.

coroa commented 1 year ago

Thanks for the quick turn around!