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

Merged
merged 4 commits into from
May 22, 2024
Merged

doc: update Java interop document #5783

merged 4 commits into from
May 22, 2024

Conversation

utamori
Copy link
Contributor

@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.

src/content/interop/java-interop.md Outdated Show resolved Hide resolved
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
Contributor 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!

@HosseinYousefi
Copy link
Member

@parlough Please merge whenever you see fit.

@parlough
Copy link
Member

/gcbrun

@dart-github-bot
Copy link
Collaborator

Visit the preview URL for this PR (updated for commit 430acaf):

https://dart-dev--pr5783-main-opirkgwc.web.app

Copy link
Member

@parlough parlough left a comment

Choose a reason for hiding this comment

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

Thank you both!

@parlough parlough merged commit 3b40f86 into dart-lang:main May 22, 2024
10 checks passed
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

4 participants