DM-10384: Refactor pybind11 wrapper code into a single module. #14
Conversation
There was a problem hiding this comment.
I'm not sure that I have much useful to say about the refactored pybind11 itself. It probably would have been more useful to have the diff be between the old and new files directly, instead of deleted and added files.
This new style is going to take some care to make sure it's clear to developers what the different blocks of code/wrappers are.
| }); | ||
| cls.def("__eq__", &SpherePoint::operator==, py::is_operator()); | ||
| cls.def("__ne__", &SpherePoint::operator!=, py::is_operator()); | ||
| /* Members */ |
There was a problem hiding this comment.
Some more whitespace to separate the various blocks would be handy in this file.
There was a problem hiding this comment.
Will do.
But just to satisfy my own curiosity: what about the extra whitespace in this context do you like? I tend to find the sections like this in the pybind11 annoying (I imagine @pschella started them and then they got copied), I suppose because I don't like things competing with the vertical whitespace between functions and classes without a very good reason. But I'm getting the impression I may be in the minority there.
There was a problem hiding this comment.
Yes, I do like white space and that is where it came from (I guess). But it is also fine with me to remove it (not that it matters what I think, but still).
There was a problem hiding this comment.
I like having some separation between logically-connected blocks of similar code. Makes it easier to find the thing you're looking for.
| cls.def("clip", &Box2D::clip); | ||
| cls.def("getCorners", &Box2D::getCorners); | ||
| cls.def("toString", &Box2D::toString); | ||
| cls.def("__repr__", [](Box2D const &self) { |
There was a problem hiding this comment.
A little more whitespace; at least separate the stringification/dunder methods from the above block for clarity.
TallJimbo
force-pushed
the
tickets/DM-10384
branch
from
dbd62ff
to
c8d3f1f
Compare
There was a problem hiding this comment.
Just skimmed it to get the overall structure. I do like the approach, even if it is a bit boilerplate heavy. It does take it a bit further away from what pybind11 wrappers normally look like, and so I'm curious what the upstream developers think of this and if they had a different solution in mind for this kind of problem. But that of course should not stop us here.
| mod.attr("ONE_OVER_PI") = py::float_(ONE_OVER_PI); | ||
| mod.attr("SQRTPI") = py::float_(SQRTPI); | ||
| mod.attr("INVSQRTPI") = py::float_(INVSQRTPI); | ||
| mod.attr("ROOT2") = py::float_(ROOT2); |
There was a problem hiding this comment.
Why not a wrapAttributes, or wrapConstants? This looks a bit confusing in a wrapFunctions block.
There was a problem hiding this comment.
That's a fair point; what I really want is one word that means "module-scope things". Maybe just wrap would be better?
| wrappers.wrapType( | ||
| PyAngle(wrappers.module, "Angle"), | ||
| [](auto & mod, auto & cls) mutable { | ||
| cls.def(py::init<double, AngleUnit>(), py::arg("val"), py::arg("units") = radians); |
There was a problem hiding this comment.
Why py::arg instead of the literals? Remnant from the past?
There was a problem hiding this comment.
Yes, just a remnant. Didn't even notice it until now.
| }); | ||
| cls.def("__eq__", &SpherePoint::operator==, py::is_operator()); | ||
| cls.def("__ne__", &SpherePoint::operator!=, py::is_operator()); | ||
| /* Members */ |
There was a problem hiding this comment.
Yes, I do like white space and that is where it came from (I guess). But it is also fine with me to remove it (not that it matters what I think, but still).
If I were them, my response would probably be, "oh, wow, why do you have so many circular dependencies? Stop doing that!" |
It doesn't just do functions.
No description provided.