Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

jjtolton
Copy link
Contributor

@jjtolton jjtolton commented Oct 21, 2022

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

@jjtolton jjtolton changed the title Cast args of py{*} family of macros to python via py/->python ISSUE-224: Cast args of py{*} family of macros to python via py/->python Oct 21, 2022
@jjtolton jjtolton changed the title ISSUE-224: Cast args of py{*} family of macros to python via py/->python ISSUE-224: Null pointer exception on Python iterator for uncast Clojure hashmap Oct 21, 2022
@behrica
Copy link
Contributor

behrica commented Oct 21, 2022

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

"__iter__" (as-tuple-instance-fn #(.iterator ^Iterable (keys (self->map %1))))

(and I had the NPE on this line)

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

@behrica
Copy link
Contributor

behrica commented Oct 21, 2022

It is easily fixed, by doing:

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

1 similar comment
@behrica
Copy link
Contributor

behrica commented Oct 21, 2022

It is easily fixed, by doing:

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

@jjtolton
Copy link
Contributor Author

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

@jjtolton jjtolton changed the title ISSUE-224: Null pointer exception on Python iterator for uncast Clojure hashmap Autocasting args of py{*} family of macros to python via py/->python Oct 21, 2022
@cnuernber
Copy link
Collaborator

cnuernber commented Oct 22, 2022

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 jjtolton closed this Oct 22, 2022
@jjtolton
Copy link
Contributor Author

@cnuernber makes good points, closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants