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

Nullsafety #586

Merged
merged 35 commits into from
Apr 13, 2021
Merged

Nullsafety #586

merged 35 commits into from
Apr 13, 2021

Conversation

okocsis
Copy link
Contributor

@okocsis okocsis commented Apr 1, 2021

Hey, guys! I've made a lot of effort to port the dart codebase to a nullsafe environment. I would appreciate someone taking a look at it.

known issues:
-not all of the test are compiling, although I've tried to make it work I've even ported some of the mocks to the new generated mocking scheme. FIXME: I think many of the test should be retired, in favour of general nullsafety. Please help me on that.
-there was a bleadapter lifecycle fiasco, because the DevicesBloc destroyed the client on dispose(), making the internal adapter unreachable, after going to detail view. Although I have attempted to fix this by just removing the destroy client call from DevicesBloc.dispose() but I'm just not certain this was the right solution in terms of good design.

Thanks for taking the time!

Fixes #556

@CLAassistant
Copy link

CLAassistant commented Apr 1, 2021

CLA assistant check
All committers have signed the CLA.

This was referenced Apr 1, 2021
@mikolak mikolak self-requested a review April 7, 2021 12:56
Copy link
Collaborator

@mikolak mikolak left a comment

Choose a reason for hiding this comment

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

Just some early feedback as I'm on 11/51 files - I'll finish it tomorrow. Looks nice so far, thanks for the PR!

.vscode/settings.json Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
lib/ble_manager.dart Show resolved Hide resolved
lib/characteristic.dart Outdated Show resolved Hide resolved
lib/characteristic.dart Show resolved Hide resolved
@mikolak mikolak added this to the 3.0.0 milestone Apr 8, 2021
Copy link
Collaborator

@mikolak mikolak left a comment

Choose a reason for hiding this comment

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

Got a few questions and a change request, but other than that looks OK!

example/ios/Runner.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
lib/characteristic.dart Show resolved Hide resolved
lib/error/ble_error.dart Outdated Show resolved Hide resolved
lib/scan_result.dart Outdated Show resolved Hide resolved
lib/scan_result.dart Outdated Show resolved Hide resolved
lib/scan_result.dart Outdated Show resolved Hide resolved
lib/src/bridge/characteristics_mixin.dart Outdated Show resolved Hide resolved
lib/src/bridge/characteristics_mixin.dart Outdated Show resolved Hide resolved
lib/src/bridge/scanning_mixin.dart Outdated Show resolved Hide resolved
test/src/bridge/lib_core_test.dart Outdated Show resolved Hide resolved
okocsis added 6 commits April 8, 2021 23:12
added final qualifiers
isConnectable is back to nullable since it's only available on iOS on android it will be null which does not mean false!!
_parseCharacteristicWithValueWithTransactionIdResponse()::
signature is now type safe
moved the rawJsonValue String checking to the caller side
@okocsis
Copy link
Contributor Author

okocsis commented Apr 8, 2021

@mikolak
Okay so, it seems the new xcode image did the trick for travis, now phase two passes as well, but still fails on phase three package-analysis as follows:
"package-analysis
It seems that this action doesn't have the required permissions to call the GitHub API with the token you gave. This can occur if this repository is a fork, as in that case GitHub reduces the GITHUB_TOKEN's permissions for security reasons. Consequently, no report will be made on GitHub. Check this issue for more information: axel-op/dart-package-analyzer#2"

I don't know how to fix this one mate.

@mikolak
Copy link
Collaborator

mikolak commented Apr 9, 2021

Yeah, it's because it's a PR from a fork instead of a MR from this repo - don't mind.

also _resetScanEvents introduced to null the underlying scream out
lib/src/bridge/scanning_mixin.dart Outdated Show resolved Hide resolved
lib/src/bridge/scanning_mixin.dart Outdated Show resolved Hide resolved
…g even if withResponse is false

additional type safety on invokeMethod calls
Copy link
Collaborator

@mikolak mikolak left a comment

Choose a reason for hiding this comment

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

I appreciate your work, but please, revert the last commit. Native client has to either return characteristic or error and I'd rather have a null pointer exception thrown than some custom exception that does not give the API consumer any insight or returning an invalid default value.

There's a bug where the response is not properly returned on Android, but it's a multilayer issue that originates in Android's MBA, any changes to interface will be done along with fixing it.

Changes to the mixins are unrelated to this PR and should be in a separate PR.

Summing up: thanks for all the work on null safety! ❤️ Please, run the formatter in scanning_mixin.dart and revert commit 86dafd6 (Characteristic:: refactor on Futures to always complete...) and I'll merge it! 🙂

lib/src/bridge/bluetooth_state_mixin.dart Outdated Show resolved Hide resolved
lib/service.dart Outdated Show resolved Hide resolved
lib/src/_managers_for_classes.dart Outdated Show resolved Hide resolved
lib/src/_managers_for_classes.dart Outdated Show resolved Hide resolved
lib/src/_managers_for_classes.dart Outdated Show resolved Hide resolved
lib/peripheral.dart Outdated Show resolved Hide resolved
lib/characteristic.dart Outdated Show resolved Hide resolved
lib/src/bridge/bluetooth_state_mixin.dart Outdated Show resolved Hide resolved
lib/src/bridge/characteristics_mixin.dart Outdated Show resolved Hide resolved
lib/src/bridge/characteristics_mixin.dart Outdated Show resolved Hide resolved
okocsis added 2 commits April 13, 2021 08:20
…something even if withResponse is false"

This reverts commit 86dafd6.
Copy link
Collaborator

@mikolak mikolak left a comment

Choose a reason for hiding this comment

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

A few minors, but I'll take care of those myself. Thank you very much for the PR!

lib/src/bridge/characteristics_mixin.dart Show resolved Hide resolved
lib/src/bridge/characteristics_mixin.dart Show resolved Hide resolved
lib/src/bridge/characteristics_mixin.dart Show resolved Hide resolved
lib/src/bridge/characteristics_mixin.dart Show resolved Hide resolved
lib/src/internal_ble_manager.dart Show resolved Hide resolved
lib/src/internal_ble_manager.dart Show resolved Hide resolved
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.

Null-Safety migration
4 participants