py2and3: Minor compatibility updates #10175
Conversation
There was a problem hiding this comment.
+@sammy-tri for both reviews, please.
Reviewable status: all discussions resolved, platform LGTM missing
There was a problem hiding this comment.
Reviewed 7 of 7 files at r1.
Reviewable status:complete! all discussions resolved, platform LGTM from [sammy-tri]
bindings/pydrake/common/test/drake_variant_pybind_test.cc, line 39 at r1 (raw file):
m.def("VariantToString", &VariantToString, py::arg("value")); py::globals().attr("update")(m.attr("__dict__"));
BTW this occurs in a few places and seems to have a strange smell to it and no comments beyond "For Python3". If I were writing a test here, I'm not sure I'd understand when this has to happen or not (but then again my pybind knowledge is not super great). Also it doesn't matter for this file since the resulting executable has only one test, but if there were multiple tests in the same file, would this leak into other tests? Or is py::globals() somehow more isolated than that? (apologies if I'm not understanding what's going on here.)
There was a problem hiding this comment.
Reviewed 7 of 7 files at r2.
Reviewable status:
EricCousineau-TRI
force-pushed
the
issue/8352_misc_20181210
branch
from
d774035 to
e3386cf
Compare
There was a problem hiding this comment.
Reviewable status:
bindings/pydrake/common/test/drake_variant_pybind_test.cc, line 39 at r1 (raw file):
Previously, sammy-tri (Sam Creasey) wrote…
BTW this occurs in a few places and seems to have a strange smell to it and no comments beyond "For Python3". If I were writing a test here, I'm not sure I'd understand when this has to happen or not (but then again my pybind knowledge is not super great). Also it doesn't matter for this file since the resulting executable has only one test, but if there were multiple tests in the same file, would this leak into other tests? Or is
py::globals()somehow more isolated than that? (apologies if I'm not understanding what's going on here.)
OK Yeah, I don't really know... Made it a utility method to make the (anti)pattern more traceable, though.
There was a problem hiding this comment.
Reviewed 5 of 5 files at r3.
Reviewable status:
EricCousineau-TRI
force-pushed
the
issue/8352_misc_20181210
branch
from
e3386cf to
707d02d
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4.
Reviewable status:
Towards #8352.
This comes primarily from #9614 (with some minor whitespace change). If you'd like to test it with Python3, this README has instructions on how to do so with that branch:
https://github.com/EricCousineau-TRI/drake/blob/feature/py3-wip/py3.README.md
This change is