[go: nahoru, domu]

Skip to content
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

Python interop #1541

Merged
merged 9 commits into from
Mar 5, 2021
Merged

Python interop #1541

merged 9 commits into from
Mar 5, 2021

Conversation

kustosz
Copy link
Contributor
@kustosz kustosz commented Mar 3, 2021

Pull Request Description

Adds Python interop.

Important Notes

  1. There's no graalpython distribution for windows :(
  2. The integration is currently suboptimal, until graalpython gets a real GIL. In the future releases, this should be done better (no context proxies).

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the Scala, Java, and Rust style guides.
  • All documentation and configuration conforms to the markdown and YAML style guides.
  • All code has been tested where possible.

@kustosz kustosz marked this pull request as ready for review March 3, 2021 15:05
Copy link
Contributor
@iamrecursion iamrecursion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A very simple implementation, overall! I want to see some changes to the tests before merge though.

spec_impl =
Test.group "Http" <|
pending = if is_ci then Nothing else """
The HTTP tests only run when the CI environment variable is set to true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The HTTP tests only run when the CI environment variable is set to true
The HTTP tests only run when the `CI` environment variable is set to true

Test.specify "should allow mutual calling of instance-level methods" <|
My_Type 3 4 . my_method_3 5 . should_equal 36

Test.specify "should expose methods and fields of JS objects" <|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Test.specify "should expose methods and fields of JS objects" <|
Test.specify "should expose methods and fields of Python objects" <|

obj.compare 5 . should_be_false
obj.compare 11 . should_be_true

Test.specify "should expose array interfaces for JS arrays" <|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Test.specify "should expose array interfaces for JS arrays" <|
Test.specify "should expose array interfaces for Python arrays" <|

Comment on lines +76 to +81
Test.specify "should make Python strings type pattern-matchable" <|
str = here.make_str "x"
t = case str of
Text -> True
_ -> False
t.should_be_true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably have a similar test for "make python numbers type pattern-matchable". Same for JS if we don't already.

import com.oracle.truffle.api.interop.UnsupportedTypeException;
import com.oracle.truffle.api.library.CachedLibrary;

@NodeField(name = "foreignFunction", type = Object.class)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you implement the JsForeignNode using @NodeField? Consistency matters for this, otherwise it might be seen as intentional or important.

Comment on lines -48 to +49
public static DataflowError withTrace(Object payload, Node location, StackTraceElement[] trace) {
var error = new DataflowError(payload, location);
error.setStackTrace(trace);
return error;
public static DataflowError withTrace(Object payload, AbstractTruffleException prototype) {
return new DataflowError(payload, prototype);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to update the Javadoc comment above as the arguments have changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ayyy true

This reverts commit 946700aff4960523d5a2e7ab9cfaa50e45481229.
@kustosz kustosz merged commit 03fa549 into main Mar 5, 2021
@kustosz kustosz deleted the wip/mk/python branch March 5, 2021 11:18
iamrecursion pushed a commit that referenced this pull request Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p-highest Should be completed ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants