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

utility function to warn changes in default arguments #5653

Closed
wyli opened this issue Dec 5, 2022 · 4 comments · Fixed by #5738
Closed

utility function to warn changes in default arguments #5653

wyli opened this issue Dec 5, 2022 · 4 comments · Fixed by #5738

Comments

@wyli
Copy link
Contributor

wyli commented Dec 5, 2022

Is your feature request related to a problem? Please describe.

def deprecated_arg(
name,
since: Optional[str] = None,
removed: Optional[str] = None,
msg_suffix: str = "",
version_val: str = __version__,
new_name: Optional[str] = None,
warning_category=FutureWarning,
):

the deprecated_arg can trigger warnings when a deprecating argument is provided by the user,
it however doesn't warn when a default value of an argument has been/will be changed.

for example it's currently not possible to warn about these changes:

@wyli wyli changed the title utility function to warn changes in default behaviour utility function to warn changes in default arguments Dec 5, 2022
@wyli wyli mentioned this issue Dec 12, 2022
7 tasks
@Shadow-Devil
Copy link
Contributor

I would like to work on this. I'm not quite sure if we want to add this functionality into deprecated_arg or have a separate decorator like deprecated_default since otherwise it's a bit ambiguos if there argument is deprecated or it's default simply changed. What do you think @wyli?

@wyli
Copy link
Contributor Author

wyli commented Dec 12, 2022

agreed, please feel free to create a separate decorator for this feature request.

@Shadow-Devil
Copy link
Contributor

Shadow-Devil commented Dec 12, 2022

My current idea how one could implement this decorator is as follows:

@deprecated_arg_default("name", old_default="test", new_default="new_name", since="1.1.0", changed="1.2.0")
def test(name: str = "test"):
    print(name)

name, old_default and new_default are all required.
The decorators only purpose is to validate if the deprecation is valid and to print a warning if version >= since and the parameter isn't directly specified,
it doesn't change the default argument by itself (would be too much "magic" in the background IMO and would be confusing if in the function signature the old default is still present, but it's changed by the decorator).

It will print different messages if since <= version < changed, version >= changed and if since and changed are not present (adopted from deprecated_default).

Two open questions I still have:

  1. Should I include an "end of warning version"? Otherwise, all future versions will print the deprecation warning if no explicit argument is used.
  2. What if a parameters default is changed multiple times? Should I also consider this scenario in my PR?

@wyli
Copy link
Contributor Author

wyli commented Dec 12, 2022

it looks good to me, there's no need to add end of warning or consider multiple changes.

we may consider another simple case of changing the default behaviour but using the same signature:

def test(name=None):
   _name = "test"
   if name is None:
-      _name = "old_default"
+      _name = "new_default"
   return _name

@wyli wyli closed this as completed in #5738 Jan 4, 2023
wyli pushed a commit that referenced this issue Jan 4, 2023
Fixes #5653.

### Description

A new utility decorator that enables us to warn users of a changing
default argument.
Current implementation does the following:
- If version < since no warning will be printed.
- If the default argument is explicitly set by the caller no warning
will be printed.
- If since <= version < replaced a warning like this will be printed:
"Default of argument `{name}` has been deprecated since version
`{since}` from `{old_default}` to `{new_default}`. It will be changed in
version `{replaced}`."
- If replaced <= version a warning like this will be printed: "Default
of argument `{name}` was changed in version `{changed}` from
`{old_default}` to `{new_default}`."
- It doesn't validate the `old_default`, so you can even use this in
scenarios where the default is actually `None` but set later in the
function. This also enables us to set `old_default` to any string if the
default is e.g. not printable.
- The only validation that will throw an error is, if the `new_default`
== the actual default and version < replaced. Which means, that somebody
replaced the value already, before the version was incremented. Apart
from that also any value for `new_default` can be set, giving the same
advantages as for the `old_default`.

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] Non-breaking change (fix or new feature that would not break
existing functionality).
- [x] New tests added to cover the changes.
- [x] Quick tests passed locally by running `./runtests.sh --quick
--unittests --disttests`.
- [ ] In-line docstrings updated.
- [ ] Documentation updated, tested `make html` command in the `docs/`
folder.

Signed-off-by: Felix Schnabel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants