Repository navigation
Renamed function argument - deprecate decorator - #5214
Conversation
|
👍 Looks good to me! But @evertrol should test to make sure that this works for his other FITS PR. |
|
Not sure but I think the test failure is unrelated. Could someone take a look and restart it, if possible? |
|
Restarted. |
|
Apologies; got distracted, and I have only now tested this. 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 (Pedantry: should we care about the parameter versus argument naming? As I had called my decorator |
|
@evertrol Thank you. I think the 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). |
Would it make sense to change the arguments for the decorator 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. |
|
The signature problem remind me this recent article: https://hynek.me/articles/decorators/ |
Well, if you have two arguments |
|
@astrofrog I've added a commit which allows to provide multiple deprecated arguments.
@saimn Interesting reading, thank you for sharing it! Maybe it's worth discussing that topic on astropy-dev? |
|
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). |
| - 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] | ||
|
|
There was a problem hiding this comment.
Could I suggest: "This can be used to rename a function argument, while it still allows for the use of the older argument name."
|
@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. |
|
@evertrol Thank you for looking over this. 👍 |
|
Rebased again. Does someone else want to review this? cc: @eteq because you are listed as the maintainer of |
|
bump |
| assert method(clobber=1) == 1 | ||
|
|
||
| assert len(w) == 1 | ||
| assert '1.3' in str(w[0].message) |
There was a problem hiding this comment.
Shouldn't these 2 lines that deal with w be within the with block?
There was a problem hiding this comment.
I've seen both styles and this specific example is copied from another test. I can indent it if you want it.
There was a problem hiding this comment.
At least to me, it is easier to read if those lines are indented, since they actually depend on w generated by the with.
There was a problem hiding this comment.
I will put them in the with ... context.
| assert test(clobber=1) == 1 | ||
|
|
||
| assert len(w) == 1 | ||
| assert '1.3' in str(w[0].message) |
There was a problem hiding this comment.
Same comment as above about lines with w should be in with block.
| assert test(clobber=1) == 1 | ||
|
|
||
| assert len(w) == 1 | ||
| assert '1.3' in str(w[0].message) |
There was a problem hiding this comment.
See above. Also applies to all subsequent occurences.
| 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): |
There was a problem hiding this comment.
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.
|
|
||
| - ``astropy.utils`` | ||
|
|
||
| - Added a new decorator: ``deprecated_renamed_argument``. This can be used to |
There was a problem hiding this comment.
Shouldn't this be under "new features" for 1.3, not "bug fixes"?
|
@pllim Thank you for the comments! I think I've adressed and pushed all of them. |
|
The sphinx build failed because it couldn't resolve Maybe their intersphinx is currently down (but there was no warning when fetching) or corrupted. Could someone restart that test in a few hours? |
|
@pllim - No problem :) |
saimn
left a comment
saimn
left a comment
There was a problem hiding this comment.
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.
|
|
||
| Parameters | ||
| ---------- | ||
| old_name : str |
There was a problem hiding this comment.
Should be str or list of str, and the same below for new_name and since.
There was a problem hiding this comment.
totally forgot to update these. 👍
| # 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. |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Especially because it's likely to fail nevertheless:
- It's unlikely that single letters of the
old_nameare 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).
There was a problem hiding this comment.
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 ;)).
There was a problem hiding this comment.
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.
|
@saimn Do you know if any library or package ( I have a similar problem with the |
|
@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/ ( |
|
The suggestion from the mailing list: https://groups.google.com/forum/#!topic/astropy-dev/VITGjHcpSa4 to use 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. 😄 |
|
bump There are still two PRs depending on this PR. Any outstanding issues or anyone else wanting to review? |
Annoying, but logical. Thanks for having tried this ! |
|
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. |
|
@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? |
|
Okay, how about squash commit number 3 to last? That would leave only 3 commits total. |
|
ok, done 👍 |
|
Close enough. 😄 I'll merge when tests pass unless I hear objections before that. |
|
@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. |
|
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. |
|
Not sure why it took so long. I restarted the build. |
|
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? |
|
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. |
|
ok, let's wait until it starts. The fork test build is mostly done (and all green). |
|
Maybe that #5214 (comment) is responsible? 😄 |
|
@pllim It passed. |
|
Ok. Merging. Thanks for your continued patience, @MSeifert04 ! |
5 participants
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)