search

clj-python / libpython-clj

Python bindings for Clojure
Eclipse Public License 2.0
1.08k stars 68 forks source link

Autocasting args of `py{*}` family of macros to python via `py/->python` #225

Closed jjtolton closed 1 year ago

jjtolton commented 1 year ago

Autocasting args of py{*} family of macros to python via py/->python

behrica commented 1 year ago

Maybe this fixes one case, but this line is still wrong:

https://github.com/clj-python/libpython-clj/blob/3c8be2fb93f08b3532118392ded4823081aa0865/src/libpython_clj2/python/bridge_as_python.clj#L125

(and I had the NPE on this line)

Whenever this line is used with an empty map, there will be an NPE.

behrica commented 1 year ago

It is easily fixed, by doing:

 "__iter__" (as-tuple-instance-fn #(.iterator ^Iterable (or (keys (self->map %1)) [] )))
behrica commented 1 year ago

It is easily fixed, by doing:

 "__iter__" (as-tuple-instance-fn #(.iterator ^Iterable (or (keys (self->map %1)) [] )))
jjtolton commented 1 year ago

Well it doesn't crash any tests so I don't see why not. I'll move the autocasting to a different issue.

cnuernber commented 1 year ago

The question here is do you always want to completely copy all data into the python and is that even possible. The system is setup to allow large objects that may not have a full python representation to be bridged. @behrica's fix still allows this while this fix will cause at least some things that did work before to fail as the entire JVM object may not be copyable to python.

The tradeoff is that copying may be both more robust and faster in a lot of cases as then Python functions down the line deal with native Python objects and not bridged objects. As it is users can manually call as-python or ->python to choose which they want but I think this fix causes the as-python pathway to attempt a complete conversion which I don't like.

jjtolton commented 1 year ago

@cnuernber makes good points, closing