This repository has been archived by the owner on Feb 22, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[shared_preferences] Migrate platform plugins to null-safety #3523
[shared_preferences] Migrate platform plugins to null-safety #3523
Changes from 1 commit
2e22820
235cef0
e19321f
e2e33ec
1119226
dad45b8
798a9b6
6cf839c
c4acbf3
827e55f
286637c
92577c9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Removed this line because it was making the test fail and by reading
SharedPreferences.registerWith(registrar)
's implementation, it always sets the instance toSharedPreferencesPlugin
.plugins/packages/shared_preferences/shared_preferences_web/lib/shared_preferences_web.dart
Lines 17 to 19 in 2e22820
Maybe I am missing something.
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.
@ditman ^^^
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.
Looking at this again: yes, it always registers itself as a
SharedPreferencesPlugin
, but this check is before the registration, making sure that the later check is actually testing something meaningful.What exactly was the error? Does this just need handling of a null instnace?
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.
True, as per here :
plugins/packages/shared_preferences/shared_preferences_platform_interface/lib/shared_preferences_platform_interface.dart
Lines 38 to 39 in 2e22820
It is supposed to return a
MethodChannelSharedPreferencesStore
but the test fails onmaster
for some reason :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.
Odd. How about instead of just removing that line, replacing it with manually setting the
instance
to a dummy value (i.e, an empty subclass created by the test) then. Otherwise in the case you are seeing, nothing is actually being tested.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 think this is testing that the instance is null until
SharedPreferencesPlugin.registerWith
is called a couple of lines below.(Maybe with null safety that part of the test is not required anymore?) We'll eventually have to revisit these tests so they run with flutter drive instead of flutter run --platform chrome. Thanks for the migration!!