-
Notifications
You must be signed in to change notification settings - Fork 75
[jextract] Add async legacy mode #462
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
Changes from all commits
65ef80b
504d1d4
b8100d9
2f4a0d4
0ec5d42
53b4bd9
e75d722
9ca8094
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,7 @@ public enum JExtractAsyncFuncMode: String, Codable { | |
| /// Android 23 and below. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please have this framed about java versions first and foremost, and about Android versions secondary.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, its not really about JDK versions either. We generate code that uses JDK 17 language features. This is just a result of Android adopting the Java library features at different times and in later API levels
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ignore android exists for a moment; this is absolutely a JDK question. Android isn't JDK so we mention the Android version, for anyone else, this is specifically about JDK versions. |
||
| /// | ||
| /// - Note: Prefer using the `completableFuture` mode instead, if possible. | ||
| // case future | ||
| case legacyFuture | ||
| } | ||
|
|
||
| extension JExtractAsyncFuncMode { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why these changes if we did not change the generation mode in this sample?
We should find a way to test both modes, we don't want to have zero coverage of either runtime functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just makes it easier to switch between the two modes when testing. These tests should work no matter what Future type is used.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we're lucky enough that these modes are source compatible on this basic level. We should make the verification process make use of that though -- in the long term we'd need source directories for specific modes, but for this specific PR how about we add another step that sets an env var, that env var then the ci-validate script picks up and changes the mode in the swift-java config.
We're growing more "modes" and it's probably time we start actually testing them, even if it's a rarely used mode like this.