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] Adds new clearWithParameters and getAllWithParameters methods to platform interface. #4261

Merged
merged 4 commits into from
Jun 27, 2023

Conversation

tarrinneal
Copy link
Contributor

Adds new clearWithParameters and getAllWithParameters methods.

part of flutter/flutter#128948

precursor to #3794

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki 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 listed 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.
  • 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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@tarrinneal tarrinneal changed the title [shared_preferences] Adds new clearWithParameters and getAllWithParameters methods. [shared_preferences] Adds new clearWithParameters and getAllWithParameters methods to platform interface. Jun 21, 2023
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.

You'll need to add temporary // ignores for the deprecation warnings in all the other packages.

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 with the shared_preferences changelog and version bump reverted.

@@ -1,3 +1,7 @@
## 2.1.3

* Ignores deprecation warnings.
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't need a release or CHANGELOG note, we just override the check with the labels (until I have time to make the script smarter). Ignores are dev-only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought so, but the bot yelled at me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, that's where the override labels come in. The bot isn't looking at lines of diff yet, only changed files.

@tarrinneal tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 22, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 22, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Jun 22, 2023

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

@stuartmorgan stuartmorgan added override: no versioning needed Override the check requiring version bumps for most changes override: no changelog needed Override the check requiring CHANGELOG updates for most changes labels Jun 22, 2023
@stuartmorgan
Copy link
Contributor

Overriding release metadata check: the changes in shared_preferences are dev-only.

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 22, 2023
@tarrinneal tarrinneal merged commit e073e55 into flutter:main Jun 27, 2023
68 of 69 checks passed
@whesse
Copy link

whesse commented Jun 27, 2023

I think this does need a version and a release, because the flutter_plugins tests check out these repositories, use pub_get to check out their dependencies, and then analyze them.

flutter_plugins is now failing on the main flutter CI, and on the monorepo CI.

@stuartmorgan
Copy link
Contributor

stuartmorgan commented Jun 27, 2023

I think this does need a version and a release, because the flutter_plugins tests check out these repositories, use pub_get to check out their dependencies, and then analyze them.

I'm not following. Why would we need to publish for tests that are operating on copies of the plugins that are cloned from repo source?

flutter_plugins is now failing on the main flutter CI, and on the monorepo CI.

Can you link to a failure? I would expect that to be the result of the pin not having rolled forward, rather than a lack of publishing, in which case the solution is just to update the pin.

@whesse
Copy link

whesse commented Jun 27, 2023

The failing builder is

https://ci.chromium.org/ui/p/flutter/builders/prod/Linux%20flutter_plugins

a failing build is:

https://ci.chromium.org/ui/p/flutter/builders/prod/Linux%20flutter_plugins/12841/overview

Another failing builder, on the Dart CI, is

https://ci.chromium.org/ui/p/dart/builders/ci.sandbox/flutter-linux-flutter-plugins

I think the failure may be due to a pinned package being chosen in the checkout, but then "flutter pub get" is run in that package to fetch its dependencies from the published versions. That is why a package pinned in flutter still needs to be resolvable and correct against its publicly available dependencies. That is what this flutter_plugins analysis test tests.

@stuartmorgan
Copy link
Contributor

The failing builder is

https://ci.chromium.org/ui/p/flutter/builders/prod/Linux%20flutter_plugins

That's definitely due to the pinned version in flutter/flutter not having rolled yet. It looks like the auto-roller is due to run in 20 minutes, or we could move the pin forward manually.

Another failing builder, on the Dart CI, is

https://ci.chromium.org/ui/p/dart/builders/ci.sandbox/flutter-linux-flutter-plugins

I don't know how the Dart CI manages its pin, but that's also checking out a version of flutter/packages from before this PR landed.

then "flutter pub get" is run in that package to fetch its dependencies from the published versions.

I'm aware of that, but we aren't running analyze against the pub get-fetched dependencies of a given package, we are analyzing the package itself, which is from source. If we published all of the higher-level packages right now, absolutely nothing would change about those failures.

That is why a package pinned in flutter still needs to be resolvable and correct against its publicly available dependencies.

They are both resolvable and correct against public dependencies. They (and again, "they" here is older versions of the packages that don't have the changes in this PR) just have analysis warnings. The solution is to update the versions being analyzed to have the // ignore directives, which means rolling the flutter/packages checkout forward in those CI systems.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 27, 2023
…llWithParameters` methods to platform interface. (flutter/packages#4261)
@whesse
Copy link

whesse commented Jun 27, 2023

OK, I understand now. I didn't see the difference between the pins of packages and of flutter_plugins, and that the package with the deprecations was being rolled separately from the package with the tests. That is why I thought one of the two must be being fetched somehow.

@stuartmorgan
Copy link
Contributor

I didn't see the difference between the pins of packages and of flutter_plugins, and that the package with the deprecations was being rolled separately from the package with the tests.

I don't follow. There's just one pin involved here, not two separate rolls.

That is why I thought one of the two must be being fetched somehow.

The package that introduced the deprecations (the one published in this PR) is being fetched via pub get in the context of the packages that are failing analysis. But that's not where the failures are, the failures are in the older copies of packages that are being analyzed from source.

fluttermirroringbot pushed a commit to flutter/flutter that referenced this pull request Jun 27, 2023
flutter/packages@6b70804...f89ce02

2023-06-27 hansmuller@google.com Updated
rfw/test/material_widgets_test.dart for M3 (flutter/packages#4316)
2023-06-27 tarrinneal@gmail.com [shared_preferences] Adds new
`clearWithParameters` and `getAllWithParameters` methods to platform
interface. (flutter/packages#4261)
2023-06-26 engine-flutter-autoroll@skia.org Roll Flutter from
042c036 to 96a2c05 (60 revisions) (flutter/packages#4313)
2023-06-26 10687576+bparrishMines@users.noreply.github.com
[file_selector_android] Create initial Android implementation of the
file_selector package (flutter/packages#3814)
2023-06-26 49699333+dependabot[bot]@users.noreply.github.com
[in_app_pur]: Bump org.json:json from 20230227 to 20230618 in
/packages/in_app_purchase/in_app_purchase/example/android/app
(flutter/packages#4244)
2023-06-26 me@ghyeok.io [webview_flutter_wkwebview] Adds the
`isInspectable` to `WebKitWebViewController`. (flutter/packages#3984)
2023-06-26 stuartmorgan@google.com [ci] Remove jcenter from legacy
project (flutter/packages#4306)

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 flutter-ecosystem@google.com,rmistry@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
tarrinneal added a commit that referenced this pull request Jun 29, 2023
…eters methods to all platforms. (#4262)

Adds new `clearWithParameters` and `getAllWithParameters` methods.

part of flutter/flutter#128948

precursor to #3794

awaiting #4261

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] 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`.)
- [x] I signed the [CLA].
- [x] The title of the PR starts with the name of the package surrounded
by square brackets, e.g. `[shared_preferences]`
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated `pubspec.yaml` with an appropriate new version according
to the [pub versioning philosophy], or this PR is [exempt from version
changes].
- [x] I updated `CHANGELOG.md` to add a description of the change,
[following repository CHANGELOG style].
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/packages/blob/main/CONTRIBUTING.md
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[relevant style guides]:
https://github.com/flutter/packages/blob/main/CONTRIBUTING.md#style
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
[pub versioning philosophy]: https://dart.dev/tools/pub/versioning
[exempt from version changes]:
https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#version-and-changelog-updates
[following repository CHANGELOG style]:
https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changelog-style
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
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 override: no changelog needed Override the check requiring CHANGELOG updates for most changes override: no versioning needed Override the check requiring version bumps for most changes p: shared_preferences platform-android platform-web
Projects
None yet
3 participants