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

✨ Adds dart fix when generating files #2182

Merged
merged 9 commits into from
Sep 9, 2024

Conversation

AlexV525
Copy link
Contributor

@AlexV525 AlexV525 commented Jul 3, 2024

Changes

Please list issues fixed by this PR here, using format "Fixes #the-issue-number".

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.

Copy link

welcome bot commented Jul 3, 2024

Hi! Thanks for opening this pull request! 😄

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jul 3, 2024

Good idea! I wonder how long does it take (will it be very slow or not), and may I know what it fixes for your scenario?

@AlexV525
Copy link
Contributor Author

AlexV525 commented Jul 3, 2024

Good idea! I wonder how long does it take (will it be very slow or not)

Probably 2~3x compared to dart format.

and may I know what it fixes for your scenario?

Yeah I'm grabbing some summary then I'll put it in the top comment. In general, people apply different lint rules in their libraries so this will help the generated files follow their rules. Also fix then format could help to solve most of the lints.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jul 3, 2024

I see. Btw, there may be another way as well: what about using // ignore_for_file: whatever_rule_name_here for the default rules, and also dart_preamble: ... for extra custom rules?

EDIT: If we want to ensure we ignore as many things as possible, we may also enable all linter rules in our frb_example/pure_dart test, then we will know what rules to ignore

@AlexV525
Copy link
Contributor Author

Do we have tests against the generated files? Like testing the dart format woks properly.

@AlexV525 AlexV525 closed this Jul 15, 2024
@AlexV525 AlexV525 reopened this Jul 15, 2024
Copy link

codecov bot commented Jul 15, 2024

Codecov Report

Attention: Patch coverage is 97.14286% with 2 lines in your changes missing coverage. Please review.

Project coverage is 99.06%. Comparing base (a44f789) to head (46e7100).
Report is 45 commits behind head on master.

Files Patch % Lines
frb_codegen/src/library/commands/dart_fix.rs 96.22% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2182      +/-   ##
==========================================
- Coverage   99.07%   99.06%   -0.01%     
==========================================
  Files         487      488       +1     
  Lines       20160    20197      +37     
==========================================
+ Hits        19974    20009      +35     
- Misses        186      188       +2     

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

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jul 15, 2024

Do we have tests against the generated files? Like testing the dart format woks properly.

Check .github/workflows/ci.yaml, iirc the CI / Ling :: Dart :: Primary will check every repo's formatting, including the generated ones.

@dbsxdbsx
Copy link
Contributor

dbsxdbsx commented Aug 3, 2024

How is this going on now? I also realize that some of the generated dart files would be changed slightly after running dart fix , like this:

PS D:\DATA\BaiduSyncdisk\project\personal\proxy> dart fix --apply
Computing fixes in proxy...
Applying fixes...

lib\rust\frb_generated.dart
  avoid_return_types_on_setters • 7 fixes

lib\rust\proxy\node.dart
  annotate_overrides • 1 fix

lib\rust\proxy\node\trojan.dart
  annotate_overrides • 7 fixes

lib\rust\proxy\node\vless.dart
  annotate_overrides • 1 fix

lib\rust\proxy\node\vmess.dart
  annotate_overrides • 1 fix

the main change is trivial like this:
image

@AlexV525
Copy link
Contributor Author

AlexV525 commented Aug 3, 2024

I didn't find spare time to update this in recent weeks, but the feature is working on this branch. You can cherry-pick these commits to your fork and manually installs it.

@AlexV525
Copy link
Contributor Author

AlexV525 commented Sep 7, 2024

@fzyzcjy
Copy link
Owner

fzyzcjy commented Sep 7, 2024

Hmm maybe just rerun it... Sometimes I do see weird flakiness

EDIT: I have triggered rerun

@AlexV525
Copy link
Contributor Author

AlexV525 commented Sep 7, 2024

Could you also retrigger the codecov workflow? It seems to be throttled 😂 https://github.com/fzyzcjy/flutter_rust_bridge/actions/runs/10748747038/job/29815531574?pr=2182

@fzyzcjy
Copy link
Owner

fzyzcjy commented Sep 7, 2024

Sure, done

@AlexV525
Copy link
Contributor Author

AlexV525 commented Sep 7, 2024

Hmm still throttled. Anything that might help with it?

@fzyzcjy
Copy link
Owner

fzyzcjy commented Sep 7, 2024

I seemed to have seen this throttling for several times... Have not searched deeply about it. Maybe you can firstly finish the pr and ready for review, and we can consider codecov before really merging.

@AlexV525
Copy link
Contributor Author

AlexV525 commented Sep 7, 2024

Okay so the PR should be good for review then.

@fzyzcjy fzyzcjy marked this pull request as ready for review September 8, 2024 00:10
@fzyzcjy
Copy link
Owner

fzyzcjy commented Sep 8, 2024

Great! I will review probably within a day

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.

Well done! Only a small nit

frb_codegen/src/library/commands/format_rust.rs Outdated Show resolved Hide resolved
@fzyzcjy
Copy link
Owner

fzyzcjy commented Sep 8, 2024

@all-contributors please add @AlexV525 for code

Copy link
Contributor

@fzyzcjy

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

@fzyzcjy
Copy link
Owner

fzyzcjy commented Sep 9, 2024

Looks great!

@fzyzcjy fzyzcjy merged commit b72240e into fzyzcjy:master Sep 9, 2024
132 of 133 checks passed
Copy link

welcome bot commented Sep 9, 2024

Hi! Congrats on merging your first pull request! 🎉

@AlexV525 AlexV525 deleted the feat/generate-run-dart-fix branch September 9, 2024 06:20
This pull request was closed.
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