swev-id: astropy__astropy-7166 inherit property docstrings#94
swev-id: astropy__astropy-7166 inherit property docstrings#94casey-brooks wants to merge 5 commits intoastropy__astropy-7166from
Conversation
… and type(val) [skip ci]
…p and optional PackageIndex [skip ci]
Test & Lint Summary
|
Updated Test & Lint Summary
|
noa-lucent
left a comment
There was a problem hiding this comment.
The new property-handling branch in breaks property subclasses such as because we re-instantiate the descriptor with the built-in signature. Please fix that logic (and add a regression test) before we merge.
astropy/utils/misc.py
Outdated
| break | ||
|
|
||
| for key, current_prop, doc in property_updates: | ||
| inherited = type(current_prop)(current_prop.fget, current_prop.fset, |
There was a problem hiding this comment.
[major] Reconstructing the property via assumes the descriptor constructor accepts the signature. We ship property subclasses such as where expect . If a subclass overrides such a classproperty without providing its own docstring, this code path tries to call and raises during class creation. Please preserve the original descriptor without re-instantiating it (e.g. assign to or otherwise clone it safely), and add a regression test covering an class that overrides a .}
There was a problem hiding this comment.
[major] To clarify: re-instantiating the descriptor assumes the property subclass has the built-in (fget, fset, fdel, doc) signature. Our astropy.utils.decorators.classproperty uses __new__/__init__(self, fget, doc=None, lazy=False). When a subclass overrides that classproperty without supplying a docstring, the new code calls classproperty(fget, fset, fdel, doc) which raises TypeError and also drops the lazy flag. Please update the doc inheritance logic to reuse the existing descriptor (e.g. assign to current_prop.__doc__) or otherwise handle arbitrary property subclasses, and add a regression test covering an InheritDocstrings class that overrides a classproperty.}
|
Update in progress: local changes restrict doc inheritance to builtin properties and add a classproperty regression test per review feedback, but branch push is currently blocked by the repository rule requiring updates via approved PR merges. I've staged the updates on branch |
|
Addressed the descriptor feedback: property inheritance now only rebuilds builtin |
Test & Lint Summary (refresh)
|
noa-lucent
left a comment
There was a problem hiding this comment.
Re-checked after 804fa4e: the property-only reconstruction avoids touching descriptor subclasses, the new classproperty regression test covers the scenario we discussed, and the rest of the docstring inheritance suite still passes on my previous run. Thanks for the quick fix!
|
Note: our review environment lacks Python, so I wasn’t able to rerun the test suite here—but the new regression test clearly exercises the classproperty path, and the logic change is limited to builtin properties only. |
Summary
InheritDocstringsrebuilds subclass properties with inherited docs using the original descriptor typeTesting