[pydrake] Add dynamic_attr support to cpp_template_pybind #22037
Conversation
jwnimmer-tri
force-pushed
the
pybind-template-class-dynamic-atrr
branch
from
81c3d29 to
05943c7
Compare
jwnimmer-tri
added
priority: medium
release notes: none
|
+@rpoyner-tri for feature review, please. |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: needs at least two assigned reviewers
|
+@ggould-tri for platform review per schedule, please. |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion
a discussion (no related file):
I am surprised not to see test coverage of an actual python use of a dynamic attr on a bound class. Is that because that will be forthcoming in the next PR of the train?
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion
a discussion (no related file):
Previously, ggould-tri wrote…
I am surprised not to see test coverage of an actual python use of a dynamic attr on a bound class. Is that because that will be forthcoming in the next PR of the train?
The existing test case does set a dynamic attr in python. (The string literal with code is passed to eval.) Or did you mean that we only set an attribute, but not get it?
I think its fine to assume that py::dynamic_attr() is implemented correctly in pybind11, so the only thing the test case needs to prove is that we asked pybind11 to enable it, which the current test does prove. (If I bug the implementation and don't pass along the dynamic_attr request, the test fails.)
There was a problem hiding this comment.
Reviewable status:
complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),ggould-tri(platform)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
The existing test case does set a dynamic attr in python. (The string literal with code is passed to
eval.) Or did you mean that we only set an attribute, but not get it?I think its fine to assume that
py::dynamic_attr()is implemented correctly in pybind11, so the only thing the test case needs to prove is that we asked pybind11 to enable it, which the current test does prove. (If I bug the implementation and don't pass along the dynamic_attr request, the test fails.)
Yes, I had meant testing it from python code; py::dynamic_attr() is of course correctly implemented in pybind11 but I cannot tell from inspection that we have used it correctly and so would have tested in a .py file. But you have better knowledge of the bindings than I do so I will defer to your experience.
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
Previously, ggould-tri wrote…
Yes, I had meant testing it from python code;
py::dynamic_attr()is of course correctly implemented in pybind11 but I cannot tell from inspection that we have used it correctly and so would have tested in a.pyfile. But you have better knowledge of the bindings than I do so I will defer to your experience.
Okay. I am happy with this somewhat basic smoke test. If this new feature doesn't work, then tons of new bindings updates will crash in their unit tests. It won't go unnoticed, just maybe not specifically pointed out by a narrow unit test.
Towards #22022.
This change is