Skip to content
This repository has been archived by the owner on Jan 28, 2024. It is now read-only.

Fix generated asset/assetId property for ffi-native #634

Merged
merged 7 commits into from
Nov 1, 2023

Conversation

nmfisher
Copy link
Contributor

@nmfisher nmfisher commented Nov 1, 2023

Using asset under ffi-native generates an incorrect asset property rather than assetId (reference), which fails to compile. This PR:

  • changes the generated annotation property from asset to assetId
  • changes the YAML key from asset to assetId (technically a breaking change to existing configs, but these were presumably broken anyway)
  • adds tests for ffi-native

  • [ Y ] I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

Copy link
Contributor

@dcharkes dcharkes left a comment

Choose a reason for hiding this comment

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

Thanks @nmfisher! ❤️

technically a breaking change to existing configs, but these were presumably broken anyway

We're bumping the version to 10.0 anyway, so that should be fine.

It might have been that people were using the deprecated @FfiNatives, but I doubt it due to the native assets feature not having landed yet.

CHANGELOG.md Outdated Show resolved Hide resolved
example/ffinative/config.yaml Outdated Show resolved Hide resolved
test/ffi_native_test/build_test_dylib.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@dcharkes dcharkes left a comment

Choose a reason for hiding this comment

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

Thanks @nmfisher! 🎸

@dcharkes dcharkes merged commit 9c3b631 into dart-archive:main Nov 1, 2023
6 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants