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

[shared_preferences] Fixes get-all when suite name is used #7335

Merged
merged 9 commits into from
Aug 12, 2024

Conversation

fertrig
Copy link
Contributor

@fertrig fertrig commented Aug 7, 2024

In shared_preferences_foundation, fixes getting all preferences when suite name is used. Bug was reading only the standard user defaults. The fix uses suite name when available.

Issues fixed by this PR:

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] page, which explains my responsibilities.
  • I read and followed the [relevant style guides] and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the [CLA].
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I [linked to at least one issue that this PR fixes] in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].
  • I updated CHANGELOG.md to add a description of the change, [following repository CHANGELOG style], or this PR is [exempt from CHANGELOG changes].
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

In shared_preferences_foundation, fixes getting all preferences
when suite name is used. Bug was reading only the standard user defaults.
The fix uses suite name when available.
Copy link
Contributor

@tarrinneal tarrinneal left a comment

Choose a reason for hiding this comment

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

If you could run the format tool and update the changelog here those last two tests will pass. Thank you for putting this together.

@@ -1,5 +1,5 @@
## NEXT
Copy link
Contributor

Choose a reason for hiding this comment

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

This will need to be changed to 2.5.1

@fertrig
Copy link
Contributor Author

fertrig commented Aug 8, 2024

@tarrinneal I think this PR is good now.

@tarrinneal
Copy link
Contributor

tarrinneal commented Aug 8, 2024

I forgot during my last review that this will need testing in the integration tests (shared_preferences_foundation/example/integration_tests/shared_preferences_tests.dart), perhaps just running the entire test suite again with suite name.

@fertrig
Copy link
Contributor Author

fertrig commented Aug 8, 2024

I added a couple of integration tests. Let me know what you think.

Also, there may be a bug with the SharedPreferencesPlugin.clear method. Since it is using dictionaryRepresentation, it may try to delete keys in the global domain.

@tarrinneal
Copy link
Contributor

I added a couple of integration tests. Let me know what you think.

Also, there may be a bug with the SharedPreferencesPlugin.clear method. Since it is using dictionaryRepresentation, it may try to delete keys in the global domain.

Did you want to test/fix that with this pr as well? (no pressure)

@fertrig
Copy link
Contributor Author

fertrig commented Aug 10, 2024

No, when I test it I'll create a separate PR.

Copy link
Contributor

@tarrinneal tarrinneal left a comment

Choose a reason for hiding this comment

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

one (required) nit and the conflict updated and it's good to go. Thanks for finding and fixing this :)

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

LGTM

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 12, 2024
Copy link
Contributor

auto-submit bot commented Aug 12, 2024

auto label is removed for flutter/packages/7335, due to - The status or check suite Mac_arm64 ios_platform_tests_shard_1 master has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 12, 2024
@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 12, 2024
@stuartmorgan
Copy link
Contributor

That failure was the known infra flake, not related to the PR.

Copy link
Contributor

auto-submit bot commented Aug 12, 2024

auto label is removed for flutter/packages/7335, due to - The status or check suite Mac_arm64 ios_platform_tests_shard_1 master has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 12, 2024
@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 12, 2024
@auto-submit auto-submit bot merged commit c0109e2 into flutter:main Aug 12, 2024
76 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 12, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 12, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 12, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Aug 12, 2024
flutter/packages@f7b1256...d9a6de8

2024-08-12 49699333+dependabot[bot]@users.noreply.github.com [camera]: Bump androidx.annotation:annotation from 1.8.1 to 1.8.2 in /packages/camera/camera_android/android (flutter/packages#7371)
2024-08-12 49699333+dependabot[bot]@users.noreply.github.com [path_provider]: Bump androidx.annotation:annotation from 1.8.1 to 1.8.2 in /packages/path_provider/path_provider_android/android (flutter/packages#7376)
2024-08-12 [email protected] [pigeon] removes restriction on number of custom types per file (flutter/packages#6840)
2024-08-12 49699333+dependabot[bot]@users.noreply.github.com [pigeon]: Bump org.jetbrains.kotlin:kotlin-gradle-plugin from 2.0.0 to 2.0.10 in /packages/pigeon/platform_tests/test_plugin/android (flutter/packages#7370)
2024-08-12 [email protected] [shared_preferences] Fixes get-all when suite name is used (flutter/packages#7335)
2024-08-12 [email protected] [flutter_adaptive_scaffold] Add expanded and extra large breakpoints (flutter/packages#7300)
2024-08-12 [email protected] Manual roll Flutter from b12d861 to 9b84701 (8 revisions) (flutter/packages#7366)
2024-08-10 [email protected] Manual roll Flutter from 76107bd to b12d861 (14 revisions) (flutter/packages#7358)
2024-08-09 [email protected] [shared_preferences] fix cast error and mutable list error with `getStringList` (flutter/packages#7355)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
DBowen33 pushed a commit to DBowen33/flutter that referenced this pull request Aug 16, 2024
flutter/packages@f7b1256...d9a6de8

2024-08-12 49699333+dependabot[bot]@users.noreply.github.com [camera]: Bump androidx.annotation:annotation from 1.8.1 to 1.8.2 in /packages/camera/camera_android/android (flutter/packages#7371)
2024-08-12 49699333+dependabot[bot]@users.noreply.github.com [path_provider]: Bump androidx.annotation:annotation from 1.8.1 to 1.8.2 in /packages/path_provider/path_provider_android/android (flutter/packages#7376)
2024-08-12 [email protected] [pigeon] removes restriction on number of custom types per file (flutter/packages#6840)
2024-08-12 49699333+dependabot[bot]@users.noreply.github.com [pigeon]: Bump org.jetbrains.kotlin:kotlin-gradle-plugin from 2.0.0 to 2.0.10 in /packages/pigeon/platform_tests/test_plugin/android (flutter/packages#7370)
2024-08-12 [email protected] [shared_preferences] Fixes get-all when suite name is used (flutter/packages#7335)
2024-08-12 [email protected] [flutter_adaptive_scaffold] Add expanded and extra large breakpoints (flutter/packages#7300)
2024-08-12 [email protected] Manual roll Flutter from b12d861 to 9b84701 (8 revisions) (flutter/packages#7366)
2024-08-10 [email protected] Manual roll Flutter from 76107bd to b12d861 (14 revisions) (flutter/packages#7358)
2024-08-09 [email protected] [shared_preferences] fix cast error and mutable list error with `getStringList` (flutter/packages#7355)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024
flutter/packages@f7b1256...d9a6de8

2024-08-12 49699333+dependabot[bot]@users.noreply.github.com [camera]: Bump androidx.annotation:annotation from 1.8.1 to 1.8.2 in /packages/camera/camera_android/android (flutter/packages#7371)
2024-08-12 49699333+dependabot[bot]@users.noreply.github.com [path_provider]: Bump androidx.annotation:annotation from 1.8.1 to 1.8.2 in /packages/path_provider/path_provider_android/android (flutter/packages#7376)
2024-08-12 [email protected] [pigeon] removes restriction on number of custom types per file (flutter/packages#6840)
2024-08-12 49699333+dependabot[bot]@users.noreply.github.com [pigeon]: Bump org.jetbrains.kotlin:kotlin-gradle-plugin from 2.0.0 to 2.0.10 in /packages/pigeon/platform_tests/test_plugin/android (flutter/packages#7370)
2024-08-12 [email protected] [shared_preferences] Fixes get-all when suite name is used (flutter/packages#7335)
2024-08-12 [email protected] [flutter_adaptive_scaffold] Add expanded and extra large breakpoints (flutter/packages#7300)
2024-08-12 [email protected] Manual roll Flutter from b12d861 to 9b84701 (8 revisions) (flutter/packages#7366)
2024-08-10 [email protected] Manual roll Flutter from 76107bd to b12d861 (14 revisions) (flutter/packages#7358)
2024-08-09 [email protected] [shared_preferences] fix cast error and mutable list error with `getStringList` (flutter/packages#7355)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: shared_preferences platform-ios platform-macos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants