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: strip attributes outside of cargo-expand #1676

Merged
merged 4 commits into from
Mar 6, 2024

Conversation

Desdaemon
Copy link
Contributor

@Desdaemon Desdaemon commented Jan 17, 2024

Changes

  • Inject a special #[cfg] only when running cargo-expand to preserve FRB attributes
  • Preserve token span information

Closes #1488

Closed #1720 (<- edited by @fzyzcjy)

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

codecov bot commented Jan 17, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 55.12%. Comparing base (9fdcce2) to head (42e559d).
Report is 1 commits behind head on master.

❗ Current head 42e559d differs from pull request most recent head 1f30aae. Consider uploading reports for the commit 1f30aae to get more accurate results

Files Patch % Lines
...n/generator/api_dart/spec_generator/class/field.rs 0.00% 10 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #1676       +/-   ##
===========================================
- Coverage   99.26%   55.12%   -44.14%     
===========================================
  Files         358      351        -7     
  Lines       14948    12703     -2245     
===========================================
- Hits        14838     7003     -7835     
- Misses        110     5700     +5590     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@fzyzcjy fzyzcjy left a comment

Choose a reason for hiding this comment

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

Looks great! Only some nits and discussions

justfile Outdated Show resolved Hide resolved
frb_macros/src/lib.rs Show resolved Hide resolved
frb_macros/src/lib.rs Outdated Show resolved Hide resolved
frb_codegen/src/library/commands/cargo_expand.rs Outdated Show resolved Hide resolved
frb_codegen/src/library/commands/cargo_expand.rs Outdated Show resolved Hide resolved
frb_codegen/src/library/commands/cargo_expand.rs Outdated Show resolved Hide resolved
@Desdaemon
Copy link
Contributor Author

I've been having difficulties running precommit as it keeps deleting everything inside flutter_via_create/integrate, wonder if it's intermittent or something more serious that needs fixing.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jan 20, 2024

I've been having difficulties running precommit as it keeps deleting everything inside flutter_via_create/integrate, wonder if it's intermittent or something more serious that needs fixing.

Curently the precommit slow logic contains:

  1. precommit-generate to run codegen for every package
  2. precommit-integrate to run flutter_rust_bridge_codegen create/integrate for flutter_via_create/integrate, which deletes the folder and copy-paste-modify the internal templates into the folder

Thus, if the template is outdated, it will be pasted and override the newly generated thing...

This is suboptimal and maybe we can improve it, what do you think?

Copy link
Owner

@fzyzcjy fzyzcjy left a comment

Choose a reason for hiding this comment

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

LGTM, only a few nits :)

@Desdaemon
Copy link
Contributor Author

Thus, if the template is outdated, it will be pasted and override the newly generated thing...

Interesting, will check tomorrow what might make the template be outdated.

@fzyzcjy fzyzcjy changed the base branch from master to feat/1676 March 6, 2024 05:17
@fzyzcjy fzyzcjy merged commit a931931 into fzyzcjy:feat/1676 Mar 6, 2024
73 of 97 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.

Let Rust Analyzer understand the marker proc macros [Bug] Minor issues/improvements for frb attribute
2 participants