[libcamera-devel] [PATCH v10 4/7] Add Python bindings

[Kieran Bingham]

Quoting Tomi Valkeinen (2022-05-10 07:04:58)
> On 09/05/2022 23:56, Kieran Bingham wrote:
> 
> > I'd like to add the following here while / when merging:
> > 
> > ================================================================================
> > 
> > diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp
> > index c983da3a94f1..09646faacd5a 100644
> > --- a/src/py/libcamera/pymain.cpp
> > +++ b/src/py/libcamera/pymain.cpp
> > @@ -17,6 +17,7 @@
> >   #include <unistd.h>
> > 
> >   #include <libcamera/base/log.h>
> > +
> >   #include <libcamera/libcamera.h>
> > 
> >   #include <pybind11/functional.h>
> > @@ -138,6 +139,14 @@ static void handleRequestCompleted(Request *req)
> >   void init_pyenums(py::module &m);
> >   void init_pyenums_generated(py::module &m);
> > 
> > +/*
> > + * The python module is not expected to be directly compatible with our usual
> > + * code style, and clang-format can't easily handle the specific styles used
> > + * with chaining here. Disable clang-format for the remainder of this module.
> > + */
> > +
> > +// clang-format off
> > +
> >   PYBIND11_MODULE(_libcamera, m)
> >   {
> >          init_pyenums(m);
> > ================================================================================
> > 
> > Any objections, or modifications to the comment?
> 
> Looks good to me.
> 
> I'd really like to use clang-format, though. It almost looks to me that 
> clang-format bugs here. E.g. first clang-format formats to this:
> 
>         pyCamera
>                 .def(
>                         "start", [](Camera &self, py::dict controls) {
>                                 return 0;
>                         },
>                         py::arg("controls") = py::dict());
> 
> I have no idea why it wants to add the line feed after .def. But I 
> remove the py::arg, and format:
> 
>         pyCamera
>                 .def(
>                         "start", [](Camera &self, py::dict controls) {
>                                 return 0;
>                         });
> 
> So no change. Then I first manually remove the line feed, i.e. 
> .def("start", ... and then format:
> 
>         pyCamera
>                 .def("start", [](Camera &self, py::dict controls) {
>                         return 0;
>                 });
> 
> Ta-da! Now it's fine. But now, adding back the py::arg, and format, it 
> goes back to the first case.
> 
> I've been fighting with clang-format before, and I have never managed it 
> to format the code quite properly. Looking at the formatted pymain.cpp, 
> I see two issues: the first is the extra line split like above, the 
> second is that it likes to create very long lines for lambdas. Neither 
> of those look like intended usual libcamera coding style, but rather 
> bugs or misconfigured rules.

If you can find a way to satiate clang-format then that's probably
preferred too - but I don't want to break the way you've styled these
indents, and I think they make sense, given the modelling used by the
pybind.

Would you prefer me to leave the clang-format on and we can ignore the
checkstyle warnings here ? Or perhaps try to improve in the future?

Or we could also try to improve in the future and then re-enable
clang-format?

--
Kieran


> 
>   Tomi