Renamed function argument - deprecate decorator #5214

MSeifert04 · 2016-08-02T22:21:34Z

This is a follow up on #5208 which was conceived in #5171.

I hope I did not mess up the commit history when trying to add @evertrol as author. (thanks @bsipocz and @pllim)

pllim · 2016-08-03T13:17:47Z

👍 Looks good to me! But @evertrol should test to make sure that this works for his other FITS PR.

MSeifert04 · 2016-08-03T19:44:51Z

Not sure but I think the test failure is unrelated. Could someone take a look and restart it, if possible?

pllim · 2016-08-04T16:45:34Z

Restarted.

MSeifert04 · 2016-08-23T00:02:43Z

@evertrol I don't think there would be issues but did you check if the decorator works for your PR #5171?

Anyone interested in doing a code review?

evertrol · 2016-08-23T04:36:55Z

Apologies; got distracted, and I have only now tested this.
This was indeed largely a matter of simply renaming the decorator to keep things working. No problems there.

There is one case where the decorator will cause a failure. This is only for Python 2, when there is already a decorator that alters the function signature (in this case @deprecated, such as for fits.table.tdump). If @deprecated_renamed_argument is placed on top of that, the "args_in_kwargs" TypeError is raised, because the new_name will not be found in arguments.
Placing @deprecated_renamed_argument below avoids this problem. I assume this is because utils.compat._funcsigs.signature is not a precise copy of the Python 3 inspect.signature function (and perhaps it simply can't be).
Arguably, there may not be a use for deprecated functions to have a keyword parameter renamed, but I just did that for consistency.
I'm noting the failure here in case someone else stumbles on a similar problem in the future.

(Pedantry: should we care about the parameter versus argument naming? As I had called my decorator replaced_parameter.)

MSeifert04 · 2016-08-23T11:20:50Z

@evertrol Thank you. I think the TypeError is actually expected because functools.wraps keeps the signature only for python 3.4 and later. It's inconvenient i.e. because that makes it impossible to deprecate multiple argument-names.

If wanted - I can replace "argument" with "parameter". I generally prefer "argument" but technically it's incorrect here (https://en.wikipedia.org/wiki/Parameter_%28computer_science%29#Parameters_and_arguments).

astrofrog · 2016-08-23T12:51:03Z

It's inconvenient i.e. because that makes it impossible to deprecate multiple argument-names.

Would it make sense to change the arguments for the decorator to allow any number of arguments to be deprecated?

pllim · 2016-08-23T13:56:58Z

to allow any number of arguments to be deprecated?

In practice, is it even a good idea to deprecate so many arguments at once? But I guess it does not hurt to have that feature anyway.

saimn · 2016-08-23T14:38:13Z

The signature problem remind me this recent article: https://hynek.me/articles/decorators/
Maybe it would be good to use one of the suggested solutions (i.e. bundling decorator.py) to fix the signatures on all Python versions, and make sure this kind of decorator can be applied in any order.
This would also fix the second point (changing several arguments) by allowing to use the decorator several times.

astrofrog · 2016-08-23T15:07:01Z

In practice, is it even a good idea to deprecate so many arguments at once? But I guess it does not hurt to have that feature anyway.

Well, if you have two arguments x_fwhm and y_fwhm you want to deprecate, we need a way to do that (that specific use case happened in the past)

MSeifert04 · 2016-08-23T22:44:04Z

@astrofrog I've added a commit which allows to provide multiple deprecated arguments.

~~But I didn't have time to update the documentation (yet).~~

@saimn Interesting reading, thank you for sharing it! Maybe it's worth discussing that topic on astropy-dev?

MSeifert04 · 2016-09-09T21:12:41Z

rebased because of merge-conflict.

If anyone wants to review this I would be very happy. Currently two other PRs depend on it (or are paused because of it).

evertrol · 2016-09-12T08:05:37Z

CHANGES.rst

+  - New decorator: ``deprecated_renamed_argument``. This can be used when
+    renaming a function argument, but still want to allow for use of
+    the older parameter name. [#5214]
+


Could I suggest: "This can be used to rename a function argument, while it still allows for the use of the older argument name."

Sounds good. 👍

evertrol · 2016-09-12T08:43:18Z

@MSeifert04 I've had a look through it, and it looks all fine to me (passing tests & pylint seems to agree). Though I'm not sure if that counts as a review.

MSeifert04 · 2016-09-13T13:43:01Z

@evertrol Thank you for looking over this. 👍

MSeifert04 · 2016-09-15T15:12:19Z

Rebased again.

Does someone else want to review this?

cc: @eteq because you are listed as the maintainer of utils.

MSeifert04 · 2016-10-06T13:04:38Z

bump

pllim

Since @saimn needs this and said this is okay for his PR. I will 👍 this with some minor comments. Would be nice for extra pair of eyes, but if not, we should merge this soon.

pllim · 2016-10-06T13:38:34Z

astropy/utils/tests/test_decorators.py

+            assert method(clobber=1) == 1
+
+        assert len(w) == 1
+        assert '1.3' in str(w[0].message)


Shouldn't these 2 lines that deal with w be within the with block?

I've seen both styles and this specific example is copied from another test. I can indent it if you want it.

At least to me, it is easier to read if those lines are indented, since they actually depend on w generated by the with.

I will put them in the with ... context.

pllim · 2016-10-06T13:40:01Z

astropy/utils/tests/test_decorators.py

+        assert test(clobber=1) == 1
+
+    assert len(w) == 1
+    assert '1.3' in str(w[0].message)


Same comment as above about lines with w should be in with block.

pllim · 2016-10-06T13:40:13Z

astropy/utils/tests/test_decorators.py

+        assert test(clobber=1) == 1
+
+    assert len(w) == 1
+    assert '1.3' in str(w[0].message)


See above. Also applies to all subsequent occurences.

pllim · 2016-10-06T13:44:06Z

astropy/utils/decorators.py

+                return LooseVersion(str(x[2]))
+
+            sorted_zipped = sorted(zip(old_name, new_name, since), key=item2)
+            for k, vals in itertools.groupby(sorted_zipped, key=item2):


Just for the ease of future maintenance by someone else, perhaps explain why there is a need to sort first and then do a grouping, both using the same keys.

pllim · 2016-10-06T13:46:21Z

CHANGES.rst

@@ -203,6 +203,10 @@ Bug Fixes

 - ``astropy.utils``

+  - Added a new decorator: ``deprecated_renamed_argument``. This can be used to


Shouldn't this be under "new features" for 1.3, not "bug fixes"?

you're right!

pllim · 2016-10-06T13:48:13Z

I mistakenly mentioned @saimn in my review comment above (but GitHub won't let me edit it). Should have been @evertrol . I apologize for the confusion.

MSeifert04 · 2016-10-06T14:02:22Z

@pllim Thank you for the comments!

I think I've adressed and pushed all of them.

MSeifert04 · 2016-10-06T14:31:55Z

The sphinx build failed because it couldn't resolve numpy classes/functions.

Maybe their intersphinx is currently down (but there was no warning when fetching) or corrupted. Could someone restart that test in a few hours?

saimn · 2016-10-06T14:34:09Z

@pllim - No problem :)

saimn

Added a few comments. I would prefer if it was possible to use the decorator multiple times instead of having lists of parameters, but it seems more difficult as functools.wraps does not preserve the signature before Python 3.5. So the currently implementation is fine.

saimn · 2016-10-06T14:35:40Z

astropy/utils/decorators.py

+
+    Parameters
+    ----------
+    old_name : str


Should be str or list of str, and the same below for new_name and since.

totally forgot to update these. 👍

saimn · 2016-10-06T14:45:59Z

astropy/utils/decorators.py

+        # Assume that new_name and since are correct (tuple/list with the
+        # appropriate length) in the spirit of the "consenting adults". But the
+        # optional parameters may not be set, so if these are not iterables
+        # wrap them.


Maybe better to check that new_name and since are also list with the correct size. Otherwise, as str are iterable, it may not fail with something like @deprecated_renamed_argument(['a', 'b'], 'alpha', '1.2') (not tested, just guessing).

I don't like these explicit checks that the user entered the right types and right amount when there is no use-case for it. I think such isinstance and len(x) = len(y) checks are defensive programming and/or code smell.

Especially because it's likely to fail nevertheless:

It's unlikely that single letters of the old_name are part of the signature.

It's likely that the "add sphinx directive" operation fails if parts of the "since" are used.

So it would just be a "more accurate error message" if I added these checks. OTOH what if new_name is a str but old_name and since are lists. That case would also fail later (because it doesn't enter the if branch here). It would require a serious amount of additional checks here to make it totally safe (and slower).

As you prefer, I agree it is difficult to check everything (and it is not something I would recommend in general) and that's why it would be much cleaner without having to support multiple arguments. But yes, it is possible to do get some unexpected behaviour: https://gist.github.com/saimn/f9f30bab65a19fc1fa5dc2492b148559 (the first one is something that can really happen I think, with variants depending on the number of arguments, the second is just for the pleasure ;)).

Ok, the LooseVersion is really liberal when comparing versions. 😄

I've included a Warning box in the parameter section but I'd prefer the current option over the "test it all" one. After all it's not really a user-facing function.

MSeifert04 · 2016-10-15T01:10:51Z

@saimn Do you know if any library or package (decorator or wrapt?) would make it possible to apply the decorator several times?

I have a similar problem with the support_nddata decorator (which also needs to access the signature) and I think bundling such a package (or make it a dependency) would be great if it fixes these annoying Python introspection issues.

saimn · 2016-10-15T21:00:09Z

@MSeifert04 - 👍 to bundle one of these packages as it could be useful for other decorators. I didn't use any of the packages listed in https://hynek.me/articles/decorators/ (boltons.funcutils (1), decorator (2), wrapt (3)), but this post gives some hint on the differences between these packages: 1 and 2 don't support class methods, 2 and 3 use a closure-less style to write the decorator, 3 seems more difficult to bundle. So, I would go for 1 or 2.

MSeifert04 · 2016-10-17T20:16:26Z

The suggestion from the mailing list: https://groups.google.com/forum/#!topic/astropy-dev/VITGjHcpSa4 to use astropy.utils.decorator.wraps instead of functools.wraps doesn't work in this case (unfortunatly).

The problem is that it keeps the signature (I thought that would be good but it's also a limitation) and therefore doesn't permit the deprecated name to be used - because that name is not part of the signature anymore.

I think the current version is the best way. 😄

MSeifert04 · 2016-10-25T13:43:10Z

bump

There are still two PRs depending on this PR. Any outstanding issues or anyone else wanting to review?

saimn · 2016-10-25T20:02:49Z

The problem is that it keeps the signature (I thought that would be good but it's also a limitation) and therefore doesn't permit the deprecated name to be used - because that name is not part of the signature anymore.

Annoying, but logical. Thanks for having tried this !

pllim · 2016-10-26T14:36:32Z

I don't see why this should not get merged after all the discussions and updates based on them. But maybe one last squash and rebase and let Travis run on the squashed version? In that case, if anyone objects too late, it won't be hard to revert the merge.

MSeifert04 · 2016-10-26T14:41:38Z

@pllim I didn't dare to squash because of the commit attributed to evertrol (because of the cooperative work on this). Any suggestions which commits to squash/keep?

pllim · 2016-10-26T14:44:09Z

Okay, how about squash commit number 3 to last? That would leave only 3 commits total.

MSeifert04 · 2016-10-26T14:48:36Z

ok, done 👍

pllim · 2016-10-26T14:51:29Z

Close enough. 😄 I'll merge when tests pass unless I hear objections before that.

pllim · 2016-10-26T14:53:56Z

@MSeifert04 , what are the two PRs depending on this one again? I only see one listed in this PR. I want to make sure that all of them are milestoned to 1.3 and not earlier.

MSeifert04 · 2016-10-26T14:55:34Z

Oups, I forgot one 😞

The PRs are #5232, #5171 (both milestoned 1.3 already)

MSeifert04 · 2016-10-26T18:30:19Z

Travis hasn't started until now, so I've cancelled the build, unfortunatly I can't restart it. It does say it's restarted but Travis is obviously fooling me.

Did someone break Travis or the hooks because the build worked on my fork.

pllim · 2016-10-26T18:31:47Z

Not sure why it took so long. I restarted the build.

MSeifert04 · 2016-10-26T18:34:29Z

The other PR hasn't started either (https://travis-ci.org/astropy/astropy/pull_requests). But just for fun I've restarted my fork build (https://travis-ci.org/MSeifert04/astropy/builds/170797977) and it started immediatly. Is something in the astropy-project (including affiliated packages) stalling the builds?

pllim · 2016-10-26T18:35:50Z

Astropy org itself has many projects. Maybe it's just due to the sheer amount of tests being run across the board? Just a guess here.

MSeifert04 · 2016-10-26T18:55:20Z

ok, let's wait until it starts. The fork test build is mostly done (and all green).

MSeifert04 · 2016-10-26T19:15:06Z

Maybe that #5214 (comment) is responsible? 😄

MSeifert04 · 2016-10-28T14:23:10Z

@pllim It passed.

pllim · 2016-10-28T14:39:31Z

Ok. Merging. Thanks for your continued patience, @MSeifert04 !

pllim added the utils label Aug 3, 2016

pllim added this to the v1.3.0 milestone Aug 3, 2016

MSeifert04 mentioned this pull request Aug 12, 2016

add ignore_nan option to median_absolute_deviation #5232

Merged

evertrol reviewed Sep 12, 2016
View reviewed changes

MSeifert04 and others added 3 commits September 15, 2016 17:04

implements a decorator for deprecated renamed function arguments

cc289ee

added docstring manipulation and changelog entry

a0a2652

added pr number [skip ci]

9ae81ee

pllim reviewed Oct 6, 2016

View reviewed changes

saimn reviewed Oct 6, 2016

View reviewed changes

This was referenced Oct 26, 2016

Allow use of JPL ephemerides in coordinate transforms and light travel time corrections #5273

Merged

Cython failures on _np_utils.pyx #5437

Closed

allow multiple deprecated arguments and some corrections.

54e898d

pllim merged commit ab0d5fb into astropy:master Oct 28, 2016

MSeifert04 deleted the deprecated-argument branch October 28, 2016 14:39

evertrol mentioned this pull request Nov 21, 2016

Replace clobber parameter with overwrite #5171

Merged

This was referenced Dec 5, 2016

Remove Inclusion of Sphinx directives in deprecate_renamed_argument decorator #5492

Merged

Add absolute tolerance when comparing FITS #4729

Merged

pllim mentioned this pull request Dec 24, 2016

Clobber & Overwrite? #5644

Closed

mirca mentioned this pull request Feb 20, 2017

add <param>_0 and <param>_fit machinery back astropy/photutils#471

Merged

		@@ -203,6 +203,10 @@ Bug Fixes

		- ``astropy.utils``

		- Added a new decorator: ``deprecated_renamed_argument``. This can be used to

Renamed function argument - deprecate decorator #5214

Renamed function argument - deprecate decorator #5214

Conversation

MSeifert04 commented Aug 2, 2016 • edited

pllim commented Aug 3, 2016

MSeifert04 commented Aug 3, 2016

pllim commented Aug 4, 2016

MSeifert04 commented Aug 23, 2016

evertrol commented Aug 23, 2016

MSeifert04 commented Aug 23, 2016 • edited

astrofrog commented Aug 23, 2016

pllim commented Aug 23, 2016

saimn commented Aug 23, 2016

astrofrog commented Aug 23, 2016

MSeifert04 commented Aug 23, 2016 • edited

MSeifert04 commented Sep 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evertrol commented Sep 12, 2016

MSeifert04 commented Sep 13, 2016

MSeifert04 commented Sep 15, 2016

MSeifert04 commented Oct 6, 2016

pllim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim Oct 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim commented Oct 6, 2016

MSeifert04 commented Oct 6, 2016 • edited

MSeifert04 commented Oct 6, 2016

saimn commented Oct 6, 2016

saimn left a comment

Choose a reason for hiding this comment

saimn Oct 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSeifert04 Oct 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSeifert04 commented Oct 15, 2016

saimn commented Oct 15, 2016

MSeifert04 commented Oct 17, 2016

MSeifert04 commented Oct 25, 2016 • edited

saimn commented Oct 25, 2016

pllim commented Oct 26, 2016

MSeifert04 commented Oct 26, 2016

pllim commented Oct 26, 2016

MSeifert04 commented Oct 26, 2016

pllim commented Oct 26, 2016

pllim commented Oct 26, 2016

MSeifert04 commented Oct 26, 2016 • edited

MSeifert04 commented Oct 26, 2016 • edited

pllim commented Oct 26, 2016

MSeifert04 commented Oct 26, 2016

pllim commented Oct 26, 2016

MSeifert04 commented Oct 26, 2016

MSeifert04 commented Oct 26, 2016

MSeifert04 commented Oct 28, 2016

pllim commented Oct 28, 2016

MSeifert04 commented Aug 2, 2016 •

edited

MSeifert04 commented Aug 23, 2016 •

edited

MSeifert04 commented Aug 23, 2016 •

edited

pllim Oct 6, 2016 •

edited

MSeifert04 commented Oct 6, 2016 •

edited

saimn Oct 6, 2016 •

edited

MSeifert04 Oct 6, 2016 •

edited

MSeifert04 commented Oct 25, 2016 •

edited

MSeifert04 commented Oct 26, 2016 •

edited

MSeifert04 commented Oct 26, 2016 •

edited