Skip to content
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

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

Merged

Conversation

coroa
Copy link
Contributor

@coroa coroa commented Jul 8, 2023

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.

  • add tests that fail without the patch
  • ensure all tests pass with pytest
  • add documentation to the relevant docstrings or pages
  • add versionadded or versionchanged directives to relevant docstrings
  • add a changelog entry if this patch changes code

@coroa coroa marked this pull request as draft July 8, 2023 22:09
@tantale
Copy link
Collaborator

tantale commented Jul 9, 2023

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
Copy link
Collaborator

tantale commented Jul 9, 2023

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
Copy link
Contributor Author

coroa commented Jul 9, 2023

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
Copy link
Contributor Author

coroa commented Jul 9, 2023

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

@coroa coroa force-pushed the add-extra_stacklevel-argument branch from 3611115 to a4d54f1 Compare July 9, 2023 12:42
@coroa coroa marked this pull request as ready for review July 9, 2023 12:43
@tantale
Copy link
Collaborator

tantale commented Jul 9, 2023

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.

Copy link
Collaborator

@tantale tantale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but I will push some minor changes (typo).

CHANGELOG.rst Outdated
@@ -18,6 +18,17 @@ and this project adheres to `Semantic Versioning <https://semver.org/spec/v2.0.0
(only in comment or documentation).


v1.2.15 (2023-??-??)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer "unreleased" instead of "2023-??-??"  😉. I'll fix that.

@@ -80,16 +79,27 @@ def __init__(
By default, the category class is :class:`~DeprecationWarning`,
you can inherit this class to define your own deprecation warning category.

:type extra_stacklevel: int
:param extra_stacklevel:
Number of additonal stacklevels to consider instrumentation rather than user code.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo "additional" with two "n" and "stack levels" with two words.

@tantale
Copy link
Collaborator

tantale commented Jul 9, 2023

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.

@tantale tantale force-pushed the add-extra_stacklevel-argument branch from a4d54f1 to 75dfbfd Compare July 9, 2023 19:40
@tantale tantale changed the title Add extra_stacklevel argument fix: add extra_stacklevel argument to better control deprecated function call references Jul 9, 2023
@tantale tantale merged commit 0e8d804 into laurent-laporte-pro:master Jul 9, 2023
20 checks passed
@coroa
Copy link
Contributor Author

coroa commented Jul 9, 2023

Thanks for the quick turn around!

@coroa coroa deleted the add-extra_stacklevel-argument branch July 9, 2023 20:10
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.

[ENH] Stacklevel offset
2 participants