Deprecate InheritDocstring and remove its usage by pllim · Pull Request #8881 · astropy/astropy

pllim · 2019-06-20T06:01:39Z

Fix #7167

TODO

Check doc build and compare with existing doc. Are property and method both inheriting properly?
Pin sphinx>=1.7. Also see Bump sphinx minversion to 1.7 sphinx-astropy#24
Add change log.
~~Exclude InheritDocstring from doctest or~~ catch the warning there.
Catch AstropyDeprecationWarning in test_inherit_docstrings().

astropy/utils/tests/test_misc.py

astropy/utils/misc.py

astropy/constants/constant.py

astropy/io/votable/tree.py

astropy/timeseries/binned.py

astropy/units/core.py

astropy/units/tests/test_quantity.py

astropy/utils/misc.py

astropy/utils/tests/test_misc.py

astropy/visualization/interval.py

mhvk · 2019-07-17T22:34:05Z

@pllim - I like this! But just to be sure, with these changes, the sphinx documentation will look the same, and if someone tries to get interactive help, that still works too, right? (From a quick check, it looks like ipython is smart enough, and help works too.)

pllim · 2019-07-18T21:56:03Z

@mhvk et al. The inheritance seems to work for the two tested cases (which I deleted from the unit tests above).

Case 1

From current master -- http://docs.astropy.org/en/latest/api/astropy.units.UnrecognizedUnit.html#astropy.units.UnrecognizedUnit.is_unity (not for future viewer, this link is no longer relevant for comparison once this PR is merged)
From this PR -- https://37694-2081289-gh.circle-artifacts.com/0/home/circleci/project/docs/_build/html/api/astropy.units.UnrecognizedUnit.html#astropy.units.UnrecognizedUnit.is_unity
The docstring in "parent" -- https://37694-2081289-gh.circle-artifacts.com/0/home/circleci/project/docs/_build/html/api/astropy.units.UnitBase.html#astropy.units.UnitBase.is_unity

Case 2

From current master -- http://docs.astropy.org/en/latest/api/astropy.units.quantity.Quantity.html#astropy.units.quantity.Quantity.argmax (not for future viewer, this link is no longer relevant for comparison once this PR is merged)
From this PR -- https://37694-2081289-gh.circle-artifacts.com/0/home/circleci/project/docs/_build/html/api/astropy.units.quantity.Quantity.html#astropy.units.quantity.Quantity.argmax
The docstring in "parent" -- https://docs.scipy.org/doc/numpy/reference/generated/numpy.ndarray.argmax.html#numpy.ndarray.argmax

mhvk · 2019-07-19T16:25:58Z

Thanks for the sanity checks!

mhvk

Looks good now! I'll leave my last comment to your discretion

mhvk · 2019-07-19T16:31:15Z

astropy/utils/misc.py

-        >>> class B(A):
-        ...     def wiggle(self):
-        ...         pass
+        >>> with warnings.catch_warnings():


If you're up for it, I'd keep the docstring as is, and rather skip doctests. But obviously no big deal

I still prefer to have the doctest run because I don't want it to break before we have the chance to completely remove it. 😄

pllim · 2019-07-19T16:55:15Z

Since this was approved, I'm going to go ahead and merge it. Thanks for the reviews, everyone!

mhvk · 2019-07-19T18:54:43Z

Yay, here's to PRs that simplify! Next, get lazyproperty in the standard library?

pllim · 2019-07-19T19:18:27Z

My attempt to build CPython from source on Windows wasn't that successful, so I am out. LOL

astrofrog · 2019-07-22T15:22:38Z

I wonder if it would be worth removing the implementation of InheritDocstrings and making it an empty class since it’s not needed, which should still maintain backward compatibility?

bsipocz · 2019-07-22T16:41:13Z

My attempt to build CPython from source on Windows wasn't that successful, so I am out. LOL

@pllim - that itself is worth an issue upstream, I recall you pointed out a few talks that proudly promote how well everything should work on Windows.

pllim · 2019-07-22T16:44:30Z

which should still maintain backward compatibility?

Only if downstream packages using Sphinx>=1.7, right? Is that guaranteed?

pllim · 2019-07-22T16:46:11Z

issue upstream (at CPython)

Hmm, I thought I heard that they are moving their issue tracker to GitHub, but I am not seeing it at https://github.com/python/cpython

bsipocz · 2019-07-22T17:22:33Z

Only if downstream packages using Sphinx>=1.7, right? Is that guaranteed?

Maybe we shouldn't quietly assume, but it would be a fair assumption after a notification about it on astropy-dev etc, 1.7 is quite old and not see a clear reason why should one limit the version of a devtool that much.

bsipocz · 2019-07-22T17:27:15Z

Hmm, I thought I heard that they are moving their issue tracker to GitHub, but I am not seeing it at https://github.com/python/cpython

oh, yes. They agreed to make the move, and this is a prime example then that issues are not get reported as it's burdensome to report to BPO.

@Mariatta - do you have an ETA for the transition from BPO to github?

Mariatta · 2019-07-22T17:32:49Z

Sorry, no ETA 😥 Some technical details of how the migration should happen are still needed to be decided. At the moment the issue tracker is still at bugs.python.org.

pllim · 2019-07-22T17:43:04Z

Hello and thanks for the quick response, @Mariatta ! Is CPython even interested in something like this? If not, there is no point going through the issues stage only to be rejected right away. Please advise. Thanks!

astropy/astropy/utils/decorators.py

Lines 697 to 717 in bc404ff

    
           class lazyproperty(property): 
        
               """ 
        
               Works similarly to property(), but computes the value only once. 
        
               This essentially memorizes the value of the property by storing the result 
        
               of its computation in the ``__dict__`` of the object instance.  This is 
        
               useful for computing the value of some property that should otherwise be 
        
               invariant.  For example:: 
        
                   >>> class LazyTest: 
        
                   ...     @lazyproperty 
        
                   ...     def complicated_property(self): 
        
                   ...         print('Computing the value for complicated_property...') 
        
                   ...         return 42 
        
                   ... 
        
                   >>> lt = LazyTest() 
        
                   >>> lt.complicated_property 
        
                   Computing the value for complicated_property... 
        
                   42 
        
                   >>> lt.complicated_property 
        
                   42

Mariatta · 2019-07-22T21:11:10Z

Hmm, lazyproperty sounds a lot like cached_property, which is new in Python 3.8 https://docs.python.org/3.8/library/functools.html?highlight=cached_property#functools.cached_property

pllim · 2019-07-22T21:21:22Z

Ooooo, it does indeed, @Mariatta . I will move this to a new Astropy issue so we can discuss this properly. I like it when problems solve themselves over time. 😄

See #9036

mhvk · 2019-07-22T21:52:52Z

Wow, yet a different reason to always try "reporting" upstream!

bsipocz · 2019-07-22T23:39:29Z

Well, this bites back for astroquery via a detour through pyvo 😞

I'll work it around easily, but maybe worth sending either an e-mail around on astropy-dev or automated issues to downstream users of InheritedDocstring.

pllim · 2019-07-23T16:31:25Z

Ops... posted a notice in mailing list at https://groups.google.com/forum/#!topic/astropy-dev/ICKNTv0O_vA

pllim added Docs utils Experimental Refactoring labels Jun 20, 2019

pllim added this to the v4.0 milestone Jun 20, 2019

This comment has been minimized.

Sign in to view

bsipocz reviewed Jul 5, 2019

View reviewed changes

astropy/utils/tests/test_misc.py Show resolved Hide resolved

astropy/utils/misc.py Show resolved Hide resolved

This comment has been minimized.

Sign in to view

pllim force-pushed the depre-inherit-docstring branch from ae22a93 to 53ee23c Compare July 17, 2019 21:36

pllim removed the Experimental label Jul 17, 2019

pllim commented Jul 17, 2019

View reviewed changes

pllim mentioned this pull request Jul 17, 2019

Bump sphinx minversion to 1.7 astropy/sphinx-astropy#23

Closed

pllim added 3 commits July 18, 2019 17:07

Deprecate InheritDocstring and remove its usage.

0214c09

Add change log. Update doctest.

83f3859

Remove extraneous metaclass. Update test.

ef205b6

pllim force-pushed the depre-inherit-docstring branch from 075ef86 to ef205b6 Compare July 18, 2019 21:21

pllim mentioned this pull request Jul 18, 2019

Bump sphinx minversion to 1.7 astropy/sphinx-astropy#24

Merged

pllim added the Ready-for-final-review label Jul 18, 2019

pllim mentioned this pull request Jul 18, 2019

Deprecate InheritDocstrings #7167

Closed

mhvk approved these changes Jul 19, 2019

View reviewed changes

pllim merged commit fdb0330 into astropy:master Jul 19, 2019

pllim deleted the depre-inherit-docstring branch July 19, 2019 16:55

pllim mentioned this pull request Jul 19, 2019

MNT: Remove deprecated InheritDocstrings #9024

Closed

ellieb mentioned this pull request Jul 22, 2019

CADC Bug fix for radius parsing astropy/astroquery#1507

Merged

pllim mentioned this pull request Jul 22, 2019

Turn InheritDocstrings into an empty class #9034

Closed

pllim mentioned this pull request Jul 22, 2019

MNT: Replace lazyproperty with cached_property in Python 3.8 #9036

Open

bsipocz mentioned this pull request Jul 23, 2019

Removing pandas dependency, using Table.read() directly astropy/astroquery#1509

Merged

mhvk mentioned this pull request Jul 23, 2019

Faster info via meta class #8998

Merged

Uh oh!

Conversation

pllim commented Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhvk commented Jul 17, 2019

Uh oh!

pllim commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Jul 19, 2019

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk Jul 19, 2019

Choose a reason for hiding this comment

Uh oh!

pllim Jul 19, 2019

Choose a reason for hiding this comment

Uh oh!

pllim commented Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Jul 19, 2019

Uh oh!

pllim commented Jul 19, 2019

Uh oh!

astrofrog commented Jul 22, 2019

Uh oh!

bsipocz commented Jul 22, 2019

Uh oh!

pllim commented Jul 22, 2019

Uh oh!

pllim commented Jul 22, 2019

Uh oh!

bsipocz commented Jul 22, 2019

Uh oh!

bsipocz commented Jul 22, 2019

Uh oh!

Mariatta commented Jul 22, 2019

Uh oh!

pllim commented Jul 22, 2019

Uh oh!

Mariatta commented Jul 22, 2019

Uh oh!

pllim commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Jul 22, 2019

Uh oh!

bsipocz commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Jul 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pllim commented Jun 20, 2019 •

edited

Loading

pllim commented Jul 18, 2019 •

edited

Loading

pllim commented Jul 19, 2019 •

edited

Loading

pllim commented Jul 22, 2019 •

edited

Loading

bsipocz commented Jul 22, 2019 •

edited

Loading