-
Notifications
You must be signed in to change notification settings - Fork 809
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.
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
Implement getattr_opt on PyAnyMethods #4978
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! I have some various suggestsions...
src/types/any.rs
Outdated
// When ptr is NULL, the attribute does not exist | ||
true => Ok(None), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks wrong; if the return value is null
then an exception is raised.
I think probably better to make the non-3.13 branch just call self.getattr(attr_name)
and then check for AttributeError
(and return None
in that case).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Passing it to self.getattr
is much cleaner and logically correct. I've revised it. Thanks
src/types/any.rs
Outdated
/// This is equivalent to the Python expression `getattr(self, attr_name, None)`, but uses | ||
/// the Python 3.13+ `PyObject_GetOptionalAttr` API for efficiency. Unlike `getattr`, it | ||
/// returns `None` if the attribute is not found instead of raising `AttributeError`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be downplay the efficiency gain, as many classes implementing __getattr__
will necessarily still be raising AttributeError
which the API is swallowing internally.
/// This is equivalent to the Python expression `getattr(self, attr_name, None)`, but uses | |
/// the Python 3.13+ `PyObject_GetOptionalAttr` API for efficiency. Unlike `getattr`, it | |
/// returns `None` if the attribute is not found instead of raising `AttributeError`. | |
/// This is equivalent to the Python expression `getattr(self, attr_name, None)`, which may | |
/// be more efficient in some cases by simply returning `None` if the attribute is not found | |
/// instead of raising `AttributeError`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks & adopted your suggestions.
@@ -1656,6 +1737,86 @@ class NonHeapNonDescriptorInt: | |||
}) | |||
} | |||
|
|||
#[test] | |||
fn test_getattr_opt() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test seems possibly more complicated than it needs to be. IMO we only need to test three cases:
- try to get an attribute which exists
- try to get an attribute which doesn't exist
- try to get an attribute from a property, which raises
ValueError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed - I've revised the unit test to test only the above 3 cases. Thanks!
Fix #4842
This PR implements
getattr_opt
as proposed in the issue using the FFI PyObject_GetOptionalAttr added in Python 3.13, and includes supports for older C API versions.