Skip to content

Update to package:code_assets #6534

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

Merged
merged 13 commits into from
May 21, 2025
Merged

Conversation

mosuem
Copy link
Contributor

@mosuem mosuem commented May 6, 2025

No description provided.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Hello @mosuem, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

This pull request updates the project to use the code_assets package instead of native_assets_cli. It also adds the hooks package as a dependency. The changes involve updating the pubspec.yaml file to reflect the new dependencies, modifying the build.dart file to use the code_assets package and import the build function from the hooks package, and updating the build_libs.dart file to use the code_assets package and add compiled_data to the default cargo features.

Highlights

  • Dependency Update: Replaces native_assets_cli with code_assets and introduces hooks as a new dependency.
  • Code Modernization: Updates import statements and removes unnecessary parameters to align with the new code_assets package.
  • Feature Enhancement: Adds compiled_data to the default cargo features in build_libs.dart.

Changelog

  • ffi/dart/hook/build.dart
    • Replaces import from package:native_assets_cli/code_assets.dart to package:code_assets/code_assets.dart.
    • Adds import for package:hooks/hooks.dart.
    • Removes os and architecture parameters from DynamicLoadingBundled.
  • ffi/dart/pubspec.yaml
    • Replaces native_assets_cli dependency with code_assets.
    • Adds hooks as a dependency.
  • ffi/dart/tool/build_libs.dart
    • Replaces import from package:native_assets_cli/code_assets.dart to package:code_assets/code_assets.dart.
    • Adds compiled_data to the default cargoFeatures.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.


Assets bundled tight,
Code transforms in the night,
Hooks now intertwine,
A new build, so fine,
Features shine ever so bright.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

The pull request updates the package:code_assets dependency and makes necessary adjustments to the code. The addition of hooks dependency and the inclusion of compiled_data cargo feature seem relevant to the project's functionality. Overall, the changes appear to be well-structured and contribute to the project's evolution.

Summary of Findings

  • Unnecessary dependency import: The import of hooks in ffi/dart/hook/build.dart seems unnecessary as it's not directly used in the file. Removing it would improve code clarity.
  • Redundant OS and Architecture parameters: The os and architecture parameters in the CodeAsset constructor are removed, which might impact the asset generation process. Need to ensure that the asset generation process still works as expected without these parameters.
  • Cargo features: The addition of compiled_data to the default cargo features seems like a good addition, but it's important to ensure that this doesn't introduce any unexpected side effects or conflicts with other features.

Merge Readiness

The pull request seems to be in good shape, but the identified issues should be addressed before merging. Specifically, the unnecessary dependency import should be removed, and the impact of removing the os and architecture parameters should be carefully evaluated. I am unable to directly approve this pull request, and other reviewers should also review this code before merging.

@mosuem
Copy link
Contributor Author

mosuem commented May 6, 2025

@robertbastian any idea why dart format fails here? Locally, it all works fine. I checked out the exact Dart version to test. Is there any copying around or something going on in the CI?

@robertbastian
Copy link
Member

Non-generated files need to be reformatted with 3.9

@mosuem
Copy link
Contributor Author

mosuem commented May 7, 2025

I don't get it...

dart format .
Formatted 4 files (0 changed) in 0.25 seconds.
dart --version
Dart SDK version: 3.9.0-63.0.dev (dev) (Sun Apr 27 17:07:04 2025 -0700) on "linux_x64"

@robertbastian
Copy link
Member

~/Downloads/dart-sdk/bin/dart format ffi/dart works for me

@mosuem
Copy link
Contributor Author

mosuem commented May 7, 2025

What does ~/Downloads/dart-sdk/bin/dart format ffi/dart --version say?

@robertbastian
Copy link
Member

Dart SDK version: 3.9.0-63.0.dev (dev) (Sun Apr 27 17:07:04 2025 -0700) on "macos_arm64"

@mosuem mosuem marked this pull request as ready for review May 12, 2025 07:44
@mosuem mosuem requested a review from a team as a code owner May 12, 2025 07:44
@mosuem
Copy link
Contributor Author

mosuem commented May 12, 2025

@munificent any idea what's going on here? Why do the same Dart versions format differently?

@munificent
Copy link

Perhaps you are running into dart-lang/dart_style#1597? Is CI set up to run dart pub get before doing the format check?

@robertbastian
Copy link
Member

Yep that's probably it!

@robertbastian robertbastian merged commit 4862d31 into unicode-org:main May 21, 2025
29 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.

3 participants