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.

Sign up for GitHub

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

Jump to bottom

fix: support for Result<T, E> with exception throwing on Dart #1325

Merged
merged 109 commits into from
Sep 17, 2023
Merged

fix: support for Result<T, E> with exception throwing on Dart #1325

merged 109 commits into from
Sep 17, 2023

Conversation

SiongSng
Copy link
Contributor

@SiongSng SiongSng commented Aug 17, 2023 •
edited by fzyzcjy
Loading

Changes

Continue #582
Fixes #533
Fixes #1070

Checklist

  • An issue to be fixed by this PR is listed above.
  • New tests are added to ensure new features are working. End-to-end tests are usually in the ./frb_example/pure_dart example, more specifically, rust/src/api.rs and dart/lib/main.dart.
  • The code generator is run and the code is formatted (via just precommit).
  • If this PR adds/changes features, documentations (in the ./book folder) are updated.
  • CI is passing.

Remark for PR creator

  • Justfile is a task runner for the command line interface that allows you to run shell commands from a file named justfile in your project directory. You can use Justfile after installing it. Note that commands written in justfile of this repository are expected to be run in bash, not cmd or powershell. Running just ... commands in cmd or powershell will produce errors as the syntax is not compatible. On Windows, you can use git bash if you have Git installed.
  • If fzyzcjy does not reply for a few days, maybe he just did not see it, so please ping him.

lattice0 added 30 commits July 17, 2022 21:25
@JustSimplyKyle
Copy link
Contributor

Something weird... happen
this is fine
while this breaks!!
which is soo weird since the breaking commit is a contributor one

@SiongSng SiongSng marked this pull request as ready for review August 19, 2023 08:08
@fzyzcjy
Copy link
Owner

fzyzcjy commented Aug 19, 2023

Great job! So maybe we should fix Valgrind before having a review?

@SiongSng
Copy link
Contributor Author

The issues with Valgrind doesn't seem to be related to this PR, since it's broken since this commit 2e01d4a

@fzyzcjy
Copy link
Owner

fzyzcjy commented Aug 19, 2023 •
edited
Loading

Ok, then let me rerun it firstly...

fzyzcjy
fzyzcjy approved these changes Aug 20, 2023
View reviewed changes
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.

Good job! Ready to merge after some nits

book/src/feature/lang_result.md Outdated Show resolved Hide resolved
frb_rust/src/rust2dart.rs Outdated Show resolved Hide resolved
frb_rust/src/rust2dart.rs Outdated Show resolved Hide resolved
frb_rust/src/wasm_bindgen_src/pool.rs Outdated Show resolved Hide resolved
frb_codegen/src/parser/ty.rs Outdated Show resolved Hide resolved
frb_codegen/src/generator/dart/ty_enum.rs Outdated Show resolved Hide resolved
frb_dart/lib/src/basic.dart Outdated Show resolved Hide resolved
frb_dart/lib/src/basic.dart Outdated Show resolved Hide resolved
frb_dart/lib/src/platform_independent.dart Outdated Show resolved Hide resolved
@SiongSng
Copy link
Contributor Author

All the nits have been fixed.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Aug 21, 2023

Looks great! I will check soon

@fzyzcjy fzyzcjy changed the title fix: support for Result<T, E> with exception throwing on Dart (Draft) fix: support for Result<T, E> with exception throwing on Dart Aug 21, 2023
@fzyzcjy fzyzcjy mentioned this pull request Aug 21, 2023
Closed
@fzyzcjy
Copy link
Owner

fzyzcjy commented Aug 21, 2023

Related: #1327

@fzyzcjy
Copy link
Owner

fzyzcjy commented Aug 21, 2023

As mentioned there, the Valgrind issue blocks this PR from merging, but I am really busy and have to squeeze some time to review this PR, let alone time for fixing that. Do you have some interest in digging a bit further about that error :)

@SiongSng
Copy link
Contributor Author

I'm also looking into this issue, I guess it's an environment change that caused the Valgrind test to fail, not directly related to these two PRs.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Aug 22, 2023

Totally agree, since the failing commit is completely unrelated

@dbsxdbsx
Copy link
Contributor

I'm also looking into this issue, I guess it's an environment change that caused the Valgrind test to fail, not directly related to these two PRs.

I guess so, because mine also failed recently.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Sep 17, 2023

I would like to take the risk and firstly merge this PR, and then fix Valgrind later... Since our Valgrind does not check a lot currently (since even simplest hello world dart program triggers invalid memory access so that part is disabled)

@fzyzcjy fzyzcjy merged commit e4f73a7 into fzyzcjy:master Sep 17, 2023
@fzyzcjy
Copy link
Owner

fzyzcjy commented Sep 17, 2023

@all-contributors please add @SiongSng for code

@allcontributors
Copy link
Contributor

@fzyzcjy

@SiongSng already contributed before to code

@fzyzcjy
Copy link
Owner

fzyzcjy commented Sep 17, 2023

@all-contributors please add @JustSimplyKyle for code

@allcontributors allcontributors bot mentioned this pull request Sep 17, 2023
Merged
@allcontributors
Copy link
Contributor

@fzyzcjy

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

@fzyzcjy fzyzcjy mentioned this pull request Sep 18, 2023
Merged
9 tasks
Czappi pushed a commit to Czappi/flutter_rust_bridge that referenced this pull request Nov 10, 2023
@fzyzcjy
Merge pull request fzyzcjy#1325 from SiongSng/result_custom_error
c5ed251 
fix: support for Result<T, E> with exception throwing on Dart
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JustSimplyKyle JustSimplyKyle JustSimplyKyle left review comments

@fzyzcjy fzyzcjy fzyzcjy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Result<(),MyCustomError> instead of anyhow?
5 participants
@SiongSng @fzyzcjy @lattice0 @JustSimplyKyle @dbsxdbsx