Python: add bindings for AddCustomContext
#1988
Conversation
senyai
force-pushed
the
senyai-py-custom-context
branch
from
afea827 to
9320f58
Compare
senyai
force-pushed
the
senyai-py-custom-context
branch
from
9320f58 to
bd27591
Compare
| @@ -180,5 +180,9 @@ NB_MODULE(_benchmark, m) { | |||
| m.def("RunSpecifiedBenchmarks", | |||
| []() { benchmark::RunSpecifiedBenchmarks(); }); | |||
| m.def("ClearRegisteredBenchmarks", benchmark::ClearRegisteredBenchmarks); | |||
| m.def("AddCustomContext", benchmark::AddCustomContext, nb::arg("key"), | |||
There was a problem hiding this comment.
Why not bind this directly as add_custom_context? That might also be IDE/completions-friendlier than the indirection by the above reassignment.
(I know you did it to match the rest of the examples, but in my opinion it's worth asking whether that convention should be kept.)
There was a problem hiding this comment.
I know of one subtle reason behind this. There's documentation for "AddCustomContext" and one can find it in __init__.py, which is easily accessible from IDE. Is it good enough reason? Maybe not. Maybe there should be good docstrings for every method. Currently, I'd give this issue low priority. For me, the lack of type annotations for most methods is much more important. And maybe the lack of format checking for (google_benchmark/benchmark.cc) in CI.
There was a problem hiding this comment.
So, my answer is: convention should NOT be kept, and change of convention is not part of this issue.
I tested it.