-
Notifications
You must be signed in to change notification settings - Fork 309
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
Add The Ability To Generate Plugins From The CLI Tool #2144
Conversation
Hi! Thanks for opening this pull request! 😄 |
@fzyzcjy could I get your thoughts on this PR when you have the chance? Not sure if I covered all the related changes for a release - if any changes are needed to adjust generation of |
Good job! Some nits
You can test any CI thing locally by copy-paste-run the |
@fzyzcjy2 Thanks for the feedback!
This should be ready for review. I was unable to get some commands with In the future, we may want to separate Also, it may be worth while to attach a dev |
Looks great! (Btw you mentioned the wrong account ;) that account is to auto publish new versions of flutter_rust_bridge). Ok, I will run the diff generation later.
IMHO we do diff testing because, we want to know whether the
That's interesting, and feel free to PR for this! |
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.
Well done, and mainly some nits!
website/docs/manual/miscellaneous/92-archived/011-library/01-overview/01-setup.md
Outdated
Show resolved
Hide resolved
This reverts commit 243e318.
Oh haha my bad.
Thanks!
Well the best way to test if it works IMHO is to actually run the code. That's what I meant by
Like flutter_rust_bridge_codegen create flutter_package --template plugin &&
flutter test flutter_package/example/integration_test/simple_test.dart &&
rm -r flutter_package (probably best run the test for each platform).
I may do that. I could get started at least, write a ticket for what needs to be done and setup rust/flutter in a Containerfile and .devcontainer. Then someone else may be best take over from there. Not super familiar with github ci or additional dev dependency issues I ran into earlier. This should be good to review again/respond to my responses. |
We have two things on ci indeed:
That looks pretty reasonable! |
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.
LGTM, only a nit!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2144 +/- ##
==========================================
- Coverage 99.08% 96.30% -2.79%
==========================================
Files 486 487 +1
Lines 19991 20029 +38
==========================================
- Hits 19809 19289 -520
- Misses 182 740 +558 ☔ View full report in Codecov by Sentry. |
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.
LGTM! I will keep it unmerged for a bit of time (maybe 1d?), since need to run codegen etc for this as mentioned above
@all-contributors please add @mcmah309 for code |
I've put up a pull request to add @mcmah309! 🎉 |
Hi! Congrats on merging your first pull request! 🎉 |
Update: #2156 (comment) |
Changes
closes #2143
Adds the
template
field toflutter_rust_bridge_codegen create --template plugin
(similar toflutter create --template plugin
) andtype
toflutter_rust_bridge_codegen integrate --template plugin
. As well as opens the door for more templates going forward.Notes For Reviewers
There appear to be a lot of code changes, but that is mainly due to
frb_codegen/assets/integration_template
being split intoshared
,app
, andplugin
. And updatingfrb_example/flutter_package
Checklist
./frb_internal precommit --mode slow
(orfast
) is run (it internal runs code generator, does auto formatting, etc)../website
folder) are updated.Remark for PR creator
./frb_internal --help
shows utilities for development.