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

feat: add preamble for dart generated files #1949

Merged
merged 9 commits into from
May 18, 2024
Merged

Conversation

Krysl
Copy link
Contributor

@Krysl Krysl commented May 17, 2024

Changes

Fixes #1948

Checklist

  • An issue to be fixed by this PR is listed above.
  • New tests are added to ensure new features are working. Please refer to this page to see how to add a test.
  • ./frb_internal precommit --mode slow (or fast) is run (it internal runs code generator, does auto formatting, etc).
  • If this PR adds/changes features, documentations (in the ./website folder) are updated.
  • CI is passing. Please refer to this page to see how to solve a failed CI.

Remark for PR creator

  • ./frb_internal --help shows utilities for development.
  • If @fzyzcjy does not reply for a few days, maybe he just did not see it, so please ping him.

Copy link

welcome bot commented May 17, 2024

Hi! Thanks for opening this pull request! 😄

@Krysl
Copy link
Contributor Author

Krysl commented May 17, 2024

I wrote it too hastily and added redundant parameters. I will clean them up.

@fzyzcjy
Copy link
Owner

fzyzcjy commented May 18, 2024

Good job! To make CI pass (and if not want to run the too slow precommit), try cd frb_codegen && UPDATE_GOLDENS=1 cargo test. It would also be great to add a test, but since this is so trivial, I can also put the tests there in the next batch.

@fzyzcjy
Copy link
Owner

fzyzcjy commented May 18, 2024

No worries about the failed Web test - looks like flaky

@fzyzcjy
Copy link
Owner

fzyzcjy commented May 18, 2024

To make a test, just add premable config in https://github.com/fzyzcjy/flutter_rust_bridge/blob/master/frb_example/pure_dart/flutter_rust_bridge.yaml, and then run

./frb_internal generate-internal-frb-example-pure-dart
./frb_internal precommit-generate

and commit the changed code

@Krysl
Copy link
Contributor Author

Krysl commented May 18, 2024

Sorry for late. the frb_internal test alwayse has some strange errors, and makes me a little impatient.

To make a test, just add premable config in https://github.com/fzyzcjy/flutter_rust_bridge/blob/master/frb_example/pure_dart/flutter_rust_bridge.yaml, ...

I've already tried adding a line of configuration there before, but the test didn't report any errors, and obviously there wasn't any verification of the generated code. Maybe I'm wrong?

@Krysl
Copy link
Contributor Author

Krysl commented May 18, 2024

Adding one line of preamble affects hundreds of generated files in pure_dart and pure_dart_pde...
Should I commit these changes?
(I'm looking for a cleaner way for testing)

@fzyzcjy
Copy link
Owner

fzyzcjy commented May 18, 2024

Anyway, then let me do it (hopefully within a day), no worries!

@Krysl
Copy link
Contributor Author

Krysl commented May 18, 2024

Sorry, I'm too slow.
I add some test in frb_codegen\test_fixtures\library\codegen\generator\api_dart\mod\simple
Hope this is the right place for test.

@fzyzcjy
Copy link
Owner

fzyzcjy commented May 18, 2024

Well, no need to be in there IMHO - just add premable config in master/frb_example/pure_dart/flutter_rust_bridge.yaml.

Adding one line of preamble affects hundreds of generated files in pure_dart and pure_dart_pde...

I am not sure what happened, but the generated code will only contain a few lines of change (the new header). Maybe it is because of failure of text formatting etc?

@Krysl
Copy link
Contributor Author

Krysl commented May 18, 2024

Well, it's your code base. If you think it's acceptable, then I'll do as you say.

Maybe it is because of failure of text formatting etc?

No text formatting issues. (It's just that I don't like that such a simple patch affects so many files.)

@fzyzcjy
Copy link
Owner

fzyzcjy commented May 18, 2024

If you think it's acceptable, then I'll do as you say.
affects hundreds of generated files in pure_dart and pure_dart_pde...
No text formatting issues. (It's just that I don't like that such a simple patch affects so many files.)

Oh I see - then your simple test looks totally reasonable and elegant! Sorry for the misundestanding, I originally thought you meant some bugs in scripts to run codegen like formatting issues.

@fzyzcjy fzyzcjy merged commit b6f1ed0 into fzyzcjy:master May 18, 2024
95 checks passed
Copy link

welcome bot commented May 18, 2024

Hi! Congrats on merging your first pull request! 🎉

@fzyzcjy
Copy link
Owner

fzyzcjy commented May 18, 2024

@all-contributors please add @Krysl for code

Copy link
Contributor

@fzyzcjy

I've put up a pull request to add @Krysl! 🎉

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.

custom ignore_for_file in generated dart file
2 participants