-
Notifications
You must be signed in to change notification settings - Fork 891
Implement auto_new attribute for #[pyclass]
#5421
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
base: main
Are you sure you want to change the base?
Conversation
|
I am not quite sure why the CI failing, I dont see any failure in the logs |
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 the PR, and sorry for the very slow review.
I think this feature makes a ton of sense, I've wanted something like this for ages. auto_new is maybe the right name, the only alternative I know is something like dataclass where this is roughly equivalent to the dataclass(init=True) option. I don't think init is correct here though, since this generates __new__.
To resolve before merge: #5284 proposes #[pyclass(default)] which would allow a constructor which takes no arguments and uses Default::default(). It might be nice to not have separate default and auto_new attributes. What if we had #[pyclass(auto_new = "from_fields")] (this PR) and #[pyclass(auto_new = "default")] ? Name and design ideas welcome.
Other open questions, potentially fine to leave for follow ups:
- We could allow
#[pyo3(default = X)]annotations on the fields to construct them. - We could consider
#[pyo3(kw_only)]at both the class and field level.
The CI failure is UI tests, you can fix with nox -s update-ui-tests:
https://github.com/PyO3/pyo3/actions/runs/17557400301/job/49865008001?pr=5421#step:19:3264
| if matches!(methods_type, PyClassMethodsType::Specialization) { | ||
| bail_spanned!(opt.span() => "`auto_new` requires the `multiple-pymethods` feature."); | ||
| } |
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.
We don't need this restriction (at the cost of a slightly more complicated implementation) - see generate_protocol_slot and how it's used for __str__.
6b530ee to
bb9959b
Compare
|
Hi, thanks for the review! In regards to the defaults-related PR, As for the followups, I would leave those to be in a separate PR and get this and the default one out first. I don't have the capacity to work on them at the moment but maybe if we open an issue someone will pick it up. I am also gonna look at the |
Perhaps we could just call it |
|
@davidhewitt in regards to @Icxolu i think it might be a little confusing for newbies reading docs to have both |
|
Good question, #5551 produces a I think |
PR for issue #5416
This introduces a new
#[pyclass]attribute calledauto_new, whereset_allis required to be used alongside it. It automatically generates a#[new]function for this class, similar to that of the python dataclass. It requires themultiple-pymethodsfeautre, as it generates its own#[pymethods]block