MRG: add routine to deprecate with from/to versions #479

matthew-brett · 2016-08-10T00:26:59Z

Add deprecation routine that allows you to specify at which version
deprecation began, and at which version deprecation will raise an error.

coveralls · 2016-08-10T00:31:46Z

Coverage decreased (-0.002%) to 96.241% when pulling b527e25 on matthew-brett:fancy-deprecations into e37cb5d on nipy:master.

codecov-io · 2016-08-10T00:49:16Z

Current coverage is 94.42% (diff: 100%)

Merging #479 into master will increase coverage by 0.15%

@@             master       #479   diff @@
==========================================
  Files           161        164     +3   
  Lines         21321      21597   +276   
  Methods           0          0          
  Messages          0          0          
  Branches       2284       2295    +11   
==========================================
+ Hits          20100      20394   +294   
+ Misses          801        783    -18   
  Partials        420        420

Powered by Codecov. Last update e37cb5d...f82e217

yarikoptic · 2016-08-10T00:51:29Z

nibabel/deprecated.py

+        @functools.wraps(func)
+        def deprecated_func(*args, **kwargs):
+            if expired:
+                raise expired_class(message)


Isn't better place for this check within outer decorator itself? So you don't wait for it to be used but indicate to developers/yourself that it is time to really remove it completely?

My worry is that someone (presumably me) will forget to remove them in time, and some code will get out into the wild in that state. In that case, then the error will only occur if they try and use the functions that they shouldn't be using, instead of completely breaking the module import.

I think all these functions have their deprecations checked, so they should generate a test error when they expire.

That was kinda the point - now codebase has methods which should have been gone awhile back. Since bookings is using them, nothing is removed. As long as you run tests on 'to be released' version (easy to do by just pushing a PR with to be released changes), you will know that those functions must be removed. I guess I will also use it eg in duecredit where releases are done from successfully built tagged pushes to github. If build/release would fail - I would know that I had to remove some

But overall out is up to you of cause! Just kinda defeats the original cause/purpose of my whining ;-)

Right - but don't forget - tests will fail as soon as the version goes over the version where deprecation ends. So, even if I release a version where I forgot to remove the function, as soon as I start the next development version, on the commit after the release, the tests will fail. That way I get to see that I forgot, without giving the user code which won't import.

Ok. That is if you do have a test for every deprecated function. I didn't look at coverage of those functions, but overall coverage is not 100% (no pun intended, coverage is exemplary high) so some might be sneaking in.
But that is again is all fine with me ;-)

yarikoptic · 2016-08-10T01:46:02Z

I guess note it is time also to annotate those which issue deprecation warning already. Paired within this PR would provide usecase testing ;-)

matthew-brett · 2016-08-10T01:47:24Z

Just doing that now - should be done in an hour or so.

coveralls · 2016-08-10T02:51:39Z

Coverage increased (+0.02%) to 96.263% when pulling b24e459 on matthew-brett:fancy-deprecations into e37cb5d on nipy:master.

coveralls · 2016-08-10T03:55:20Z

Coverage increased (+0.04%) to 96.28% when pulling 385e68b on matthew-brett:fancy-deprecations into e37cb5d on nipy:master.

coveralls · 2016-08-10T04:14:04Z

Coverage increased (+0.05%) to 96.289% when pulling dd14a0b on matthew-brett:fancy-deprecations into e37cb5d on nipy:master.

matthew-brett · 2016-08-10T05:37:36Z

OK - I believe all deprecation warnings are covered by the tests, and are versioned.

coveralls · 2016-08-10T06:27:23Z

Coverage increased (+0.1%) to 96.351% when pulling 00d4e18 on matthew-brett:fancy-deprecations into e37cb5d on nipy:master.

coveralls · 2016-08-10T06:54:24Z

Coverage increased (+0.1%) to 96.351% when pulling 39fbd6b on matthew-brett:fancy-deprecations into e37cb5d on nipy:master.

coveralls · 2016-08-10T09:20:25Z

Coverage increased (+0.1%) to 96.351% when pulling 26e1ab4 on matthew-brett:fancy-deprecations into e37cb5d on nipy:master.

effigies · 2016-08-11T15:43:26Z

nibabel/filebasedimages.py

+    @deprecate_with_version('get_header method is deprecated.\n'
+                            'Please use the ``img.header`` property '
+                            'instead.',
+                            '2.1', '4.0')


This was at least deprecated in the docs at 2.0.0. Is this meant to indicate the first release a warning was raised?

Yes, I was assuming that no-one was reading the docs, and so dated the deprecation from the first warning.

effigies · 2016-08-11T15:44:38Z

Should be deprecating SpatialImage.get_affine.

effigies · 2016-08-11T16:01:40Z

Weird: I'm seeint EcatImage overrode SpatialImage.from_filespec, so it was not raising deprecation warnings. Probably should be now?

Maybe it should be removed on a shorter time-scale than 4.0, though, since it's pretty unlikely people would be using that at this point.

effigies · 2016-08-11T16:06:32Z

nibabel/spatialimages.py

+    @deprecate_with_version('get_shape method is deprecated.\n'
+                            'Please use the ``img.shape`` property '
+                            'instead.',
+                            '1.2', '3.0')
    def get_shape(self):
        """ Return shape for image

        This function deprecated; please use the ``shape`` property instead


I think deprecate_with_version adds to the docstring?

Good point.

matthew-brett · 2016-08-11T19:21:37Z

Good catch with from_filespec in ECAT. I am sure few people are using it, but I'd prefer to be gentle in removing, there isn't much cost to keeping it another version.

effigies · 2016-08-11T19:32:13Z

nibabel/filebasedimages.py

+    @deprecate_with_version('get_header method is deprecated.\n'
+                            'Please use the ``img.header`` property '
+                            'instead.',
+                            '2.1', '4.0')
    def get_header(self):
        """ Get header from image

        Please use the `header` property instead of `get_header`; we will
        deprecate this method in future versions of nibabel.


Another docstring warning.

coveralls · 2016-08-11T19:33:35Z

Coverage increased (+0.1%) to 96.358% when pulling 51e12ae on matthew-brett:fancy-deprecations into e37cb5d on nipy:master.

effigies · 2016-08-11T19:34:55Z

nibabel/loadsave.py

+@deprecate_with_version('read_img_data deprecated.'
+                        'Please use ``img.dataobj.get_unscaled()`` instead.'
+                        '2.0.1',
+                        '4.0')
 def read_img_data(img, prefer='scaled'):


This function also has a docstring deprecation notice.

coveralls · 2016-08-12T02:21:44Z

Coverage increased (+0.1%) to 96.374% when pulling b2512e9 on matthew-brett:fancy-deprecations into e37cb5d on nipy:master.

matthew-brett · 2016-08-12T02:30:13Z

I refactored the deprecation warning machinery - now in nibabel/deprecator.py - any chance of another quick review from the assembly?

coveralls · 2016-08-12T02:32:57Z

Coverage increased (+0.1%) to 96.374% when pulling 638a6ad on matthew-brett:fancy-deprecations into e37cb5d on nipy:master.

effigies · 2016-08-12T03:01:50Z

nibabel/deprecated.py

+    pass
+
+
+deprecate_with_version = Deprecator(cmp_pkg_version)


Did you want the warning class to be VisibleDeprecationWarning?

I wasn't planning on it. Numpy uses these for deprecations where the user is probably making a coding error. We could promote all our deprecations, but that might be noisy.

Gotcha. Just didn't see where it was being used. I'll have a more thorough review of the whole thing, now I'm awake again.

effigies · 2016-08-12T13:55:24Z

Review complete.

Add function to compare our package version to others. We'll use this to see whether the deprecations have expired.

Add deprecation class and function that allows you to specify at which version deprecation began, and at which version deprecation will raise an error.

Check that dev version doesn't trigger error from deprecation decorator.

Replace `np.deprecate_with_doc` with `deprecate_with_version`. There are other deprecations around the code-base that could be signalled with this decorator.

It was already broken, and has been deprecated since version 1.0.

Specify version at which DeprecationWarning was first raised, and planned version for removal. Add tests for some deprecations that were not being tested.

Thanks to Chris M for reminding me.

ECAT overriding deprecated (now removed) from_filespec method in SpatialImages. Deprecate pending removal.

matthew-brett · 2016-08-12T18:51:42Z

Thanks for the review - I've extended the docstrings / comments a bit to make it clearer what I was trying to do.

coveralls · 2016-08-12T18:54:46Z

Coverage increased (+0.1%) to 96.374% when pulling f82e217 on matthew-brett:fancy-deprecations into e37cb5d on nipy:master.

effigies · 2016-08-15T13:51:50Z

Got it. LGTM.

matthew-brett · 2016-08-15T16:21:15Z

Thanks a lot for the review.

matthew-brett mentioned this pull request Aug 10, 2016

fully deprecate (i.e. remove) some RFed years ago methods #477

Closed

yarikoptic reviewed Aug 10, 2016
View reviewed changes

matthew-brett force-pushed the fancy-deprecations branch from b527e25 to b24e459 Compare August 10, 2016 02:46

matthew-brett force-pushed the fancy-deprecations branch from b24e459 to 385e68b Compare August 10, 2016 03:50

matthew-brett force-pushed the fancy-deprecations branch from dd14a0b to ca51667 Compare August 10, 2016 05:36

matthew-brett force-pushed the fancy-deprecations branch from ca51667 to 00d4e18 Compare August 10, 2016 06:22

matthew-brett force-pushed the fancy-deprecations branch from 00d4e18 to 39fbd6b Compare August 10, 2016 06:47

matthew-brett force-pushed the fancy-deprecations branch from 39fbd6b to 26e1ab4 Compare August 10, 2016 07:15

effigies reviewed Aug 11, 2016
View reviewed changes

matthew-brett force-pushed the fancy-deprecations branch from 26e1ab4 to 51e12ae Compare August 11, 2016 19:21

effigies reviewed Aug 11, 2016
View reviewed changes

matthew-brett force-pushed the fancy-deprecations branch from 51e12ae to f1e660d Compare August 11, 2016 19:43

matthew-brett force-pushed the fancy-deprecations branch from 5bc5904 to b2512e9 Compare August 12, 2016 02:17

matthew-brett force-pushed the fancy-deprecations branch from b2512e9 to 638a6ad Compare August 12, 2016 02:28

matthew-brett changed the title ~~WIP: add routine to deprecate with from/to versions~~ MRG: add routine to deprecate with from/to versions Aug 12, 2016

effigies reviewed Aug 12, 2016
View reviewed changes

matthew-brett added 8 commits August 12, 2016 11:49

NF: add cmp_pkg_version function and test

87ac814

Add function to compare our package version to others. We'll use this to see whether the deprecations have expired.

NF+TST: add class / function to deprecate with versions

b7448eb

Add deprecation class and function that allows you to specify at which version deprecation began, and at which version deprecation will raise an error.

TST: add test of dev version for deprecation

e8654dc

Check that dev version doesn't trigger error from deprecation decorator.

RF: replace numpy deprecation decorator with ours

09c63da

Replace `np.deprecate_with_doc` with `deprecate_with_version`. There are other deprecations around the code-base that could be signalled with this decorator.

RF: remove from_filespec classmethod

1e35fc4

It was already broken, and has been deprecated since version 1.0.

RF+TST: specify versions for DeprecationWarnings

edbdd2c

Specify version at which DeprecationWarning was first raised, and planned version for removal. Add tests for some deprecations that were not being tested.

RF+TST: deprecate get_affine method of image

101af68

Thanks to Chris M for reminding me.

RF+TST: add deprecation to ECAT from_filespec

f82e217

ECAT overriding deprecated (now removed) from_filespec method in SpatialImages. Deprecate pending removal.

matthew-brett force-pushed the fancy-deprecations branch from 638a6ad to f82e217 Compare August 12, 2016 18:49

matthew-brett merged commit 20cc0fc into nipy:master Aug 15, 2016

This was referenced Aug 15, 2016

RF/ENH: remove file prior writing in to_filename #466

Open

Support for BrainVoyager VTC, MSK, VMR and NR-VMP files #216

Open

WIP: CIFTI2 spec reader and writer #249

Merged

effigies mentioned this pull request Aug 15, 2016

Resolve conflicts from #479 satra/nibabel#15

Closed

MRG: add routine to deprecate with from/to versions #479

MRG: add routine to deprecate with from/to versions #479

Conversation

matthew-brett commented Aug 10, 2016

coveralls commented Aug 10, 2016 • edited Loading

codecov-io commented Aug 10, 2016 • edited Loading

Current coverage is 94.42% (diff: 100%)

Choose a reason for hiding this comment

matthew-brett Aug 10, 2016 • edited Loading

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

yarikoptic commented Aug 10, 2016

matthew-brett commented Aug 10, 2016

coveralls commented Aug 10, 2016 • edited Loading

coveralls commented Aug 10, 2016 • edited Loading

coveralls commented Aug 10, 2016 • edited Loading

matthew-brett commented Aug 10, 2016

coveralls commented Aug 10, 2016 • edited Loading

coveralls commented Aug 10, 2016 • edited Loading

coveralls commented Aug 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Aug 11, 2016

effigies commented Aug 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-brett commented Aug 11, 2016

Choose a reason for hiding this comment

coveralls commented Aug 11, 2016 • edited Loading

Choose a reason for hiding this comment

coveralls commented Aug 12, 2016 • edited Loading

matthew-brett commented Aug 12, 2016

coveralls commented Aug 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Aug 12, 2016

matthew-brett commented Aug 12, 2016

coveralls commented Aug 12, 2016 • edited Loading

effigies commented Aug 15, 2016

matthew-brett commented Aug 15, 2016

coveralls commented Aug 10, 2016 •

edited

Loading

codecov-io commented Aug 10, 2016 •

edited

Loading

matthew-brett Aug 10, 2016 •

edited

Loading

coveralls commented Aug 10, 2016 •

edited

Loading

coveralls commented Aug 10, 2016 •

edited

Loading

coveralls commented Aug 10, 2016 •

edited

Loading

coveralls commented Aug 10, 2016 •

edited

Loading

coveralls commented Aug 10, 2016 •

edited

Loading

coveralls commented Aug 10, 2016 •

edited

Loading

coveralls commented Aug 11, 2016 •

edited

Loading

coveralls commented Aug 12, 2016 •

edited

Loading

coveralls commented Aug 12, 2016 •

edited

Loading

coveralls commented Aug 12, 2016 •

edited

Loading