ImageAPITest.test_every_method_attribute_has_docstring is meant to check that every method and attribute in the protocol has a docstring on the backend under test. It does not check properties. An overridden image_labels or catalog_labels with no docstring passes silently.
Cause
The test looks each name up on the instance:
for method in all_methods_and_attributes:
attr = getattr(self.image, method)
if not attr.__doc__:
method_attrs_no_docs.append(method)For a method that returns a bound method, whose __doc__ is the function's docstring. For a property it returns the property's value. image_labels and catalog_labels return a tuple, so the test inspects tuple.__doc__, which is always truthy.
Reproduction
from astro_image_display_api.image_viewer_logic import ImageViewerLogic
class NoDoc(ImageViewerLogic):
@property
def image_labels(self):
return tuple(self._images)
def save(self, filename, overwrite=False, **kwargs):
pass
n = NoDoc()
type(getattr(n, "image_labels")) # tuple, __doc__ is truthy -> passes the test
NoDoc.image_labels.__doc__ # None -> should have failed
n.save.__doc__ # None -> correctly caughtSo the undocumented save override is caught and the undocumented image_labels override is not.
Possible fix, and the trade-off
Looking the name up on the class, getattr(type(self.image), method), gives the property object, whose __doc__ is the getter's docstring. That handles both methods and properties.
The catch is backends that provide image_labels or catalog_labels as a plain instance attribute set in __init__. The protocol declares them as properties, but the runtime isinstance check only requires the name to exist, so such backends pass today. A class-level lookup would raise AttributeError for them. Options include:
- look up on the class first and fall back to the instance when the class has no such attribute, skipping the docstring requirement for plain data attributes;
- or decide that these two must be properties and let the class-level lookup enforce it.
Context
Found while reviewing #123, which narrows the wording in the backend guide to match what the test does today: #123 (comment). docs_from_image_viewer_logic_if_missing already handles properties correctly, so only the test is affected. Note that #105 reports a second copy of this test under tests/; any fix should cover both copies, or land after that duplicate is removed.
— Written by Claude at @mwcraig's direction.
ImageAPITest.test_every_method_attribute_has_docstringis meant to check that every method and attribute in the protocol has a docstring on the backend under test. It does not check properties. An overriddenimage_labelsorcatalog_labelswith no docstring passes silently.Cause
The test looks each name up on the instance:
For a method that returns a bound method, whose
__doc__is the function's docstring. For a property it returns the property's value.image_labelsandcatalog_labelsreturn atuple, so the test inspectstuple.__doc__, which is always truthy.Reproduction
So the undocumented
saveoverride is caught and the undocumentedimage_labelsoverride is not.Possible fix, and the trade-off
Looking the name up on the class,
getattr(type(self.image), method), gives thepropertyobject, whose__doc__is the getter's docstring. That handles both methods and properties.The catch is backends that provide
image_labelsorcatalog_labelsas a plain instance attribute set in__init__. The protocol declares them as properties, but the runtimeisinstancecheck only requires the name to exist, so such backends pass today. A class-level lookup would raiseAttributeErrorfor them. Options include:Context
Found while reviewing #123, which narrows the wording in the backend guide to match what the test does today: #123 (comment).
docs_from_image_viewer_logic_if_missingalready handles properties correctly, so only the test is affected. Note that #105 reports a second copy of this test undertests/; any fix should cover both copies, or land after that duplicate is removed.— Written by Claude at @mwcraig's direction.