Skip to content
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

Introduce support for per field instance default values. #20949

Closed
wants to merge 5 commits into from

Conversation

kaos
Copy link
Member

@kaos kaos commented May 23, 2024

Part 1 of 2 to address #20947 (Part 2: #20950)

This adds support for overriding the class var field default value per field instance.

}

fn cls_alias(cls: &PyAny) -> PyResult<&str> {
// TODO: All of these methods should use interned attr names.
Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed ^^ while I was at it..

// context to call it.
let slf = self_.borrow_mut().into_ptr();
let attr = name.into_py(py).into_ptr();
let res = ffi::PyObject_GenericGetAttr(slf, attr);
Copy link
Member Author

@kaos kaos May 23, 2024

Choose a reason for hiding this comment

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

This call (along with how to handle the returned value) was taken from https://github.com/PyO3/pyo3/blob/dc4f114d6772337d8b1568c6dbb0bf35deaa84dd/src/impl_/pyclass.rs#L212

@kaos kaos requested a review from huonw May 23, 2024 11:40
@kaos
Copy link
Member Author

kaos commented May 23, 2024

I've no idea what I'm doing here.. it was a simple enough thing in Python but I've been pulling my hair for hours to get this to work in Rust using pyo3.. (due to me lacking Rust proficiency..)

Comment on lines +246 to +258
unsafe {
// The ffi::PyObject_GenericGetAttr() call is unsafe, so we need to be in an unsafe
// context to call it.
let slf = self_.borrow_mut().into_ptr();
let attr = name.into_py(py).into_ptr();
let res = ffi::PyObject_GenericGetAttr(slf, attr);
if res.is_null() {
Err(PyErr::fetch(py))
} else {
Ok(res)
}
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@kaos kaos mentioned this pull request May 23, 2024
kaos added a commit that referenced this pull request May 24, 2024
Hoping to be able to leverage `Bound` for #20949
@benjyw
Copy link
Contributor

benjyw commented May 24, 2024

As I commented on the issue, this (and the other change) seems like a lot of extra complexity, just to conserve a "feature" that I think is pretty silly to begin with... Maybe instead we do the "alexandrian solution" of just getting rid of that silly error? :)

@kaos
Copy link
Member Author

kaos commented May 27, 2024

As discussed on #20947 we opt to change the behavior for source globs instead of having a conditional on the value being default or not, so we no longer need to be able to track overriding defaults per field instance.

@kaos kaos closed this May 27, 2024
@kaos kaos deleted the kaos-field-default-value branch May 27, 2024 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants