-
Notifications
You must be signed in to change notification settings - Fork 16
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
- Updated library to null-safety #21
Conversation
AlexBacich
commented
Apr 25, 2021
- Fixed tests
- Fixed example with tests
- Overall cleanup and small bugfix
- Fixed tests - Fixed example with tests - Overall cleanup and small bugfix
@escamoteur , please review and publish. I've fixed and run all unit \ functional tests in the library and example so that might be good to go without thorough review. |
could you check if your PR still works with the latest now safe version of RX command 6.0.0-null-safety.3? |
Done, all tests passed, updated PR |
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.
I left some comments.
Thanks a lot for your work!!!!
lib/src/rx_command_builder.dart
Outdated
class RxCommandBuilder<R> extends StatelessWidget { | ||
final Stream<CommandResult<dynamic, R>> commandResults; | ||
final RxBuilder<R>? dataBuilder; | ||
final ErrorBuilder<Exception>? errorBuilder; |
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.
Wouldn't it make sense to pass the full CommandError
to the errorBuilder
? On that note I just wonder if not all error handlers/builders should be defined to expect CommandErrors
.
I know this would be a major breaking change, but the ability to access the paramData
is a huge advantage to display meaningful error messages
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.
This makes sense, didn't see it before. I think will use it.
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.
Updated. I don't think we should define CommandErrors in all builders as this one is specific to rxCommand.
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.
Please review. all changes.
- Updated rx_command_builder.dart: made it work with CommandError
@escamoteur , ping |
did you also update the readme? |
Good point. I will update with the next update. |
Just fixed the other open issue with #19 and pushed V4.0.1 |