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

Prepare for utf8.encode() to return more precise Uint8List type #4342

Merged
merged 3 commits into from Jun 30, 2023

Conversation

mkustermann
Copy link
Member

To avoid analyzer warnings when utf8.encode() will return the more precise Uint8List type, we use const Utf8Encoder().convert() which already returns Uint8List

See dart-lang/sdk#52801

To avoid analyzer warnings when `utf8.encode()` will return the more
precise `Uint8List` type, we use `const Utf8Encoder().convert()` which
already returns `Uint8List`

See dart-lang/sdk#52801
@mkustermann mkustermann changed the title Prepare for utf8.decode() to return more precise Uint8List type Prepare for utf8.encode() to return more precise Uint8List type Jun 29, 2023
@mkustermann
Copy link
Member Author

After skimming through this (enormously long) wiki page, it's still unclear why this PR would need a version + changelog update (as it's simply changing internals to use an API which avoids an explicit downcast - nothing that would affect/concern an end user).

The packages/image_picker/image_picker_for_web/example isn't even published to pub, it doesn't have version number or changelog. Nothing to do there.

For packages/rfw: Have added a line to CHANGELOG.md & updated pubspec.yaml now.

@stuartmorgan
Copy link
Contributor

After skimming through this (enormously long) wiki page

If you have suggestions for shortening the section I linked to without removing useful information, please let me know.

it's still unclear why this PR would need a version + changelog update

I think the last item in the version exemption list may have been unclear, I've adjusted it. Does that help, or do you still believe the rfw change meets one of the exemption criteria? If so, please let me know which one and why so I can clarify further.

The packages/image_picker/image_picker_for_web/example isn't even published to pub, it doesn't have version number or changelog.

That's not strictly true; packages/image_picker/image_picker_for_web/example/lib/main.dart is prominently displayed on pub.dev. But the specific file you changed isn't published (and the CI is aware of that, so doesn't flag it).

@mkustermann
Copy link
Member Author

I think the last item in the version exemption list may have been unclear, I've adjusted it. Does that help, or do you still believe the rfw change meets one of the exemption criteria? If so, please let me know which one and why so I can clarify further.

I think of the change in itself falls into a category of changes that a) are not urgent to get published b) have no real user-visible effect. Other things that may fall into this category may be e.g. tiny perf/memory improvements, better code quality, formatting of code, smaller refactoring, ... None of those are very urgent to be published as they have little user impact, also users may have no interest in knowing about them (good to keep high signal-to-noise ratio in CHANGELOG).

That being said: The reason for the PR is a related change to Dart SDK's utf8.decode() which will make this package have analyzer flags. So maybe that justifies increasing the number / getting it published soon.

But the specific file you changed isn't published (and the CI is aware of that, so doesn't flag it).

Our dart-flutter CI does flag that as an analyzer error when doing the Dart SDK change to utf8.decode(). See this build failure:

Running command: "dart analyze --fatal-infos" in /b/s/w/ir/x/t/flutter_packages.XGEKCL/packages/image_picker/image_picker_for_web
Analyzing image_picker_for_web...

   info - example/integration_test/image_picker_for_web_test.dart:16:25 - Unnecessary cast. Try removing the cast. - unnecessary_cast
   info - example/integration_test/image_picker_for_web_test.dart:17:30 - Unnecessary cast. Try removing the cast. - unnecessary_cast

If you have suggestions for shortening the section I linked to without removing useful information, please let me know.

My preference would be to lean on as little process as possible (lowering the barrier for contributors) e.g.:

  • Update CHANGELOG.md if users of the package may have an interest in knowing what the PR changes (e.g. breaking change, new apis, larger perf/memory improvements, bug fixes, ...)
  • Update pubspec.yaml' major, minor and patch according to semantic version standard. If the change should be published soon, ensure to update the semver in the PR - otherwise it's ok to keep existing version (i.e. future PRs that increase version (and cause publish of package) will include the change)

@stuartmorgan Is there anything I should change before the PR can be approved?

@stuartmorgan
Copy link
Contributor

I think the last item in the version exemption list may have been unclear, I've adjusted it. Does that help, or do you still believe the rfw change meets one of the exemption criteria? If so, please let me know which one and why so I can clarify further.

I think of the change in itself falls into a category of changes that a) are not urgent to get published b) have no real user-visible effect. Other things that may fall into this category may be e.g. tiny perf/memory improvements, better code quality, formatting of code, smaller refactoring, ... None of those are very urgent to be published as they have little user impact, also users may have no interest in knowing about them (good to keep high signal-to-noise ratio in CHANGELOG).

I think you're answering a different question than I asked; it seems like you are arguing why you personally wouldn't version it if the policy were to leave it to everyone's individual judgement, I'm asking what part of the documentation (if any) is still unclear that it isn't exempt according to our actual policy.

But the specific file you changed isn't published (and the CI is aware of that, so doesn't flag it).

Our dart-flutter CI does flag that as an analyzer error when doing the Dart SDK change to utf8.decode()

I'm talking about the CI check that was failing when you were missing a version bump for rfw, where you were changing production code, not the analyzer:
https://cirrus-ci.com/task/5195956182843392

The CI was correctly only flagging that rfw needed a version and changelog change.

If you have suggestions for shortening the section I linked to without removing useful information, please let me know.

My preference would be to lean on as little process as possible (lowering the barrier for contributors) e.g.:

  • Update CHANGELOG.md if users of the package may have an interest in knowing what the PR changes (e.g. breaking change, new apis, larger perf/memory improvements, bug fixes, ...)
  • Update pubspec.yaml' major, minor and patch according to semantic version standard. If the change should be published soon, ensure to update the semver in the PR - otherwise it's ok to keep existing version (i.e. future PRs that increase version (and cause publish of package) will include the change)

That's not shortening the section without removing information, that's rewriting the policy.

The reason that we have the policy that's written there now is because when we had a policy like the one you are describing, it had worse outcomes and more manual overhead.

@stuartmorgan Is there anything I should change before the PR can be approved?

It already was approved (and is now passing CI). Were you looking for additional review?

@ditman ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 30, 2023
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup!

@auto-submit auto-submit bot merged commit cbbc2fb into flutter:main Jun 30, 2023
70 checks passed
@mkustermann
Copy link
Member Author

@ditman Thanks for approving & merging!

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 3, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 3, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 3, 2023
flutter/packages@53ed5a0...cdae854

2023-07-01 engine-flutter-autoroll@skia.org Roll Flutter from ff838bc to aa5f4a2 (29 revisions) (flutter/packages#4363)
2023-07-01 stuartmorgan@google.com [tool] Add a flag to skip cleanup (flutter/packages#4357)
2023-07-01 stuartmorgan@google.com [file_selector] Endorse Android (flutter/packages#4329)
2023-07-01 stuartmorgan@google.com [google_sign_in] Convert Android to Pigeon (flutter/packages#4344)
2023-06-30 tarrinneal@gmail.com [Pigeon] readme updates (flutter/packages#3705)
2023-06-30 43054281+camsim99@users.noreply.github.com [camera_android] Support concurrently image capture and image streaming (flutter/packages#4332)
2023-06-30 kustermann.martin@gmail.com Prepare for utf8.encode() to return more precise Uint8List type (flutter/packages#4342)
2023-06-30 tarrinneal@gmail.com [shared_preferences] Adds allowList to setPrefix method. (flutter/packages#3794)
2023-06-30 engine-flutter-autoroll@skia.org Roll Flutter from 51bef1b to ff838bc (12 revisions) (flutter/packages#4346)

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
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: image_picker p: rfw Remote Flutter Widgets platform-web
Projects
None yet
4 participants