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

doc: update Java interop document #5783

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

utamori
Copy link

@utamori utamori commented May 8, 2024

Pure Dart binding is now standard
This page needs to be updated so that newcomers are not confused

  • Upgrade JNIGEN to v0.9.0
  • Remove legacy C bindings

HosseinYousefi/jnigen_example#2


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
  • This PR doesn't contain automatically generated corrections or text (Grammarly, LLMs, and similar).
  • This PR follows the Google Developer Documentation Style Guidelines — for example, it doesn't use i.e. or e.g., and it avoids I and we (first person).
  • This PR uses semantic line breaks of 80 characters or fewer.
Contribution guidelines:
  • See our contributor guide for general expectations for PRs.
  • Larger or significant changes should be discussed in an issue before creating a PR.
  • Code changes should generally follow the Dart style guide and use dart format.
  • Updates to code excerpts indicated by <?code-excerpt need to be updated in their source .dart file as well.

Copy link
Member

@HosseinYousefi HosseinYousefi left a comment

Choose a reason for hiding this comment

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

There are other mentions of the C bindings still available in the text. Please also change them.

@@ -37,7 +37,6 @@ that uses `package:jnigen` to generate bindings for a simple class.

- JDK
- [Maven][]
- (Optional) [`clang-format`][] to format the generated C bindings

[Maven]: https://maven.apache.org/
[`clang-format`]: https://clang.llvm.org/docs/ClangFormat.html
Copy link
Member

Choose a reason for hiding this comment

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

This can also be removed.

Copy link
Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@HosseinYousefi HosseinYousefi left a comment

Choose a reason for hiding this comment

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

I mentioned that there are other places where the C bindings are not removed.

However I cannot point you to them, as GitHub doesn't let me comment on unchanged lines.

It would be nice to allow changes, so we can commit to your fork.

So I'll try to review using links to the lines:

https://github.com/dart-lang/site-www/pull/5783/files#diff-ea7b67b16b35f54455c4e13a54b0e18a2645749b60ad4e960f94b288061e967fR88

-To generate the Dart (and C) bindings, run `jnigen` and
+To generate the Dart bindings, run `jnigen` and

https://github.com/dart-lang/site-www/pull/5783/files#diff-ea7b67b16b35f54455c4e13a54b0e18a2645749b60ad4e960f94b288061e967fL123

-you must build the dynamic libraries for `jni` and the generated C files. 
+you must build the dynamic library for `jni`. 

@utamori
Copy link
Author

utamori commented May 15, 2024

@HosseinYousefi
I had missed it. Thanks for pointing that out.
If there's anything else, feel free to have it edited.

@HosseinYousefi HosseinYousefi self-requested a review May 15, 2024 06:25
Copy link
Member

@HosseinYousefi HosseinYousefi left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @utamori!

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.

None yet

2 participants