SkyCoord: fix misleading AttributeError for subclass properties (swev-id: astropy__astropy-14096)#16
Conversation
…criptors\n\nWhen a SkyCoord subclass defines a property whose getter raises AttributeError (e.g., accessing a missing attribute on self), Python falls back to __getattr__, which then raises an AttributeError for the original attribute name, masking the root cause.\n\nAdd an early guard in __getattr__ to detect class-level attributes (incl. properties) via the MRO and access them via object.__getattribute__ so that any AttributeError from the descriptor propagates unchanged.\n\nFixes #14.
…serves inner missing attribute message
|
Requesting review. Focus: ensure early guard in SkyCoord.getattr does not interfere with dynamic alias/frame attributes; confirm test coverage and style. PR includes both fix and unit test. |
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Summary
- Correctness: The early MRO guard in SkyCoord.getattr correctly detects class-level attributes (including subclass properties) and dispatches to object.getattribute, allowing a property’s own AttributeError to propagate unmodified. This preserves dynamic alias/frame attribute and transform-name lookup behavior because those names are not present in any class dict and will continue through the existing SkyCoord.getattr logic. This also avoids recursion and remains compatible if subclasses override getattr.
- Performance: The MRO walk is negligible relative to the existing logic. A tiny micro-optimization could check type(self).dict first, but not required.
- Tests: The new test is focused, follows project style, and asserts the desired behavior (mentioning the inner missing attribute and not the property name). It should fail prior to this change and pass after. Optionally, consider a small follow-up test to assert that dynamic alias/frame lookups still behave as before (non-blocking).
- Style/Maintenance: There is a stray duplicate triple-quoted docstring block left in getattr after the guard. It is not the function docstring and should be removed to avoid confusion and minor overhead.
Requested change before merge
- Remove the duplicated triple-quoted docstring block immediately after the early guard in getattr. Keep only the top function docstring.
Once that is addressed, this looks good to merge.
There was a problem hiding this comment.
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.
| """ | ||
| # If this attribute exists on the class (e.g., a subclass property), | ||
| # try to access it via normal attribute lookup so that any | ||
| # AttributeError raised by the descriptor propagates unchanged. |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| def test_subclass_property_inner_attributeerror_message(): | ||
| c = CustomCoord('00h42m30s', '+41d12m00s', frame='icrs') | ||
| with pytest.raises(AttributeError) as excinfo: | ||
| _ = c.prop |
There was a problem hiding this comment.
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.
|
Addressed review feedback: removed duplicated inline docstring block in getattr. Please re-review; if no further concerns, kindly approve. |
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Looks good. The duplicate inline docstring is removed, and the early MRO guard in SkyCoord.getattr cleanly preserves subclass property AttributeErrors without affecting dynamic alias/frame or transform lookups. The included test is adequate and consistent with project style. Approving.
There was a problem hiding this comment.
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.
2 participants
Fixes #14.
Problem
Root cause
Solution
Tests
Notes