Skip to content

SkyCoord: fix misleading AttributeError for subclass properties (swev-id: astropy__astropy-14096) #16

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rowan-stein wants to merge 3 commits into from
Closed

SkyCoord: fix misleading AttributeError for subclass properties (swev-id: astropy__astropy-14096) #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d90dcdf
SkyCoord.__getattr__: preserve inner AttributeError from subclass des…
Oct 19, 2025
e1a913d
Add unit test to ensure SkyCoord subclass property AttributeError pre…
Oct 19, 2025
95acd43
Remove duplicated inline docstring block from __getattr__ after addin…
Oct 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions astropy/coordinates/sky_coordinate.py
View file
Open in desktop
Copy link

@noa-lucent noa-lucent Oct 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The early MRO guard looks correct and preserves dynamic alias/frame attribute and transform lookups (those names won’t be found in any class dict, so normal SkyCoord getattr logic still runs). Using object.getattribute here is appropriate to avoid recursion and to let descriptor AttributeErrors propagate unchanged.

One nit: there is a duplicated docstring block immediately after the guard (a standalone triple-quoted string). That stray literal isn’t used as the function docstring and should be removed to avoid confusion and minor overhead. Please delete the second triple-quoted block and keep only the top docstring.

Copy link

@noa-lucent noa-lucent Oct 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick follow-up. The duplicate inline docstring has been removed. The early MRO guard remains concise and correct, and dynamic attribute behavior is preserved.

Original file line number Diff line number Diff line change
Expand Up @@ -867,10 +867,12 @@ def _is_name(self, string):
)

def __getattr__(self, attr):
"""
Overrides getattr to return coordinates that this can be transformed
to, based on the alias attr in the primary transform graph.
"""
# If this attribute exists on the class (e.g., a subclass property),
# try to access it via normal attribute lookup so that any
Copy link

@noa-lucent noa-lucent Oct 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edge case sanity check: if a subclass overrides getattr itself, this early return still delegates to object.getattribute for attributes defined in the class dict, which is correct and avoids recursion. For attributes not in any class dict, control falls through to SkyCoord.getattr to handle dynamic names as before. This matches the intended behavior.

# AttributeError raised by the descriptor propagates unchanged.
Copy link

@noa-lucent noa-lucent Oct 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iterating the full MRO is fine here and should be negligible, but to micro-optimize you could first check type(self).__dict__ before walking the rest of the MRO since subclass properties are expected to be defined on the immediate class. Not required—just a minor perf consideration.

for cls in type(self).mro():
if attr in getattr(cls, "__dict__", {}):
return object.__getattribute__(self, attr)
if "_sky_coord_frame" in self.__dict__:
if self._is_name(attr):
return self # Should this be a deepcopy of self?
Expand Down
19 changes: 19 additions & 0 deletions astropy/coordinates/tests/test_sky_coord_subclass_attr.py
View file
Open in desktop
Copy link

@noa-lucent noa-lucent Oct 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good targeted test. It follows project style, uses pytest idioms, and asserts on error message content. Consider naming the test a bit more explicitly (e.g., test_subclass_property_attributeerror_is_not_masked) but current name is acceptable.

Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Licensed under a 3-clause BSD style license - see LICENSE.rst
import pytest
import astropy.units as u
from astropy.coordinates import SkyCoord

class CustomCoord(SkyCoord):
@property
def prop(self):
# Access a non-existent attribute to trigger AttributeError from the descriptor
return self.random_attr


def test_subclass_property_inner_attributeerror_message():
c = CustomCoord('00h42m30s', '+41d12m00s', frame='icrs')
with pytest.raises(AttributeError) as excinfo:
_ = c.prop
Copy link

@noa-lucent noa-lucent Oct 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: You could also assert that accessing a non-existent dynamic attribute (e.g., a known transform alias or frame attr) still behaves as before to guard against regressions. For example, verify that a valid alias still resolves and an unknown name still raises the standard SkyCoord AttributeError. Not blocking for this fix-focused PR.

# Ensure the error message mentions the missing inner attribute, not the property name
assert "random_attr" in str(excinfo.value)
assert "prop" not in str(excinfo.value)