Implementing a few switch statements #150946

nate-thegrate · 2024-06-27T18:25:14Z

I really like how patterns can be used for variable assignment and avoiding duplicated logic. (related: #150942)

// before
final GestureRecognizer? recognizer = info.recognizer;
if (recognizer is TapGestureRecognizer) {
  if (recognizer.onTap != null) {
    configuration.onTap = recognizer.onTap;
    configuration.isLink = true;
  }
} else if (recognizer is DoubleTapGestureRecognizer) {
  if (recognizer.onDoubleTap != null) {
    configuration.onTap = recognizer.onDoubleTap;
    configuration.isLink = true;
  }
}


// after
switch (info.recognizer) {
  case TapGestureRecognizer(onTap: final VoidCallback? onTap):
  case DoubleTapGestureRecognizer(onDoubleTap: final VoidCallback? onTap):
    if (onTap != null) {
      configuration.onTap = onTap;
      configuration.isLink = true;
    }
}

victorsanni

LGTM logic wise.

Style-wise, I personally think Class(:parameter), while less verbose, can be a little disorienting to read than just Class(parameter: parameter). I think it's in the same spirit as the section of the style guide to avoid anonymous parameter names.

victorsanni · 2024-07-01T18:34:14Z

dev/tools/localization/bin/gen_date_localizations.dart

+      return buffer.toString();
+    case Map<String, dynamic>():
+      final StringBuffer buffer = StringBuffer('<String, Object>{');
+      for (final MapEntry<String, dynamic>(:String key, :dynamic value) in json.entries) {


I personally found this much harder to read than the simple forEach used previously.

avoid_function_literals_in_foreach_calls is one of Dart/Flutter's recommended linter rules.

However, it's not mentioned in the Flutter repo style guide, and you make a good point about readability. (Not to mention the fact that I wasn't consistent between this function and the one below 😅)

I opened dart-lang/sdk#55709 a couple of months ago, and if/when that's resolved we won't need duplicate type annotations.

Until then, I agree that using forEach is the best way to go.

victorsanni · 2024-07-01T18:50:34Z

packages/flutter/lib/src/rendering/editable.dart

-            if (recognizer.onTap != null) {
-              configuration.onTap = recognizer.onTap;
+        switch (info.recognizer) {
+          case TapGestureRecognizer(:final VoidCallback? onTap):


Same here as below. It works, but I feel it would be easier for me reading with onTap: .

I have somewhat mixed feelings about this—the avoid anonymous parameter names is about removing a name entirely, whereas in this situation, it's removing a duplicate name from the same line.

But with regard to this function, adding onTap: would make it parallel to the case right below it, which I agree is an improvement for readability. I'll definitely update this one.

victorsanni · 2024-07-01T19:12:00Z

It sort of reminds me of how Python has quirky inbuilt tips and tricks that serve to reduce verbosity but make it much harder to read and non-obvious to a newcomer.

victorsanni

I can imagine someone new to Dart seeing it and thinking "Dart is one of those obscure quirky languages and I don't understand what's going on".

victorsanni · 2024-07-01T20:24:11Z

packages/flutter/lib/src/widgets/editable_text.dart

+      case ScrollEndNotification() when _dataWhenToolbarShowScheduled!.value != _value:
+        _dataWhenToolbarShowScheduled = null;
+        _disposeScrollNotificationObserver();
+      case ScrollNotification(:final BuildContext? context)


Here, for instance, I missed the colon on first glance and thought it was just case ScrollNotification(final BuildContext? context) (doesn't help that : and final have the same color highlight). It took a minute to read carefully again before I understood what was going on.

But maybe it's just me. I am new to Dart myself :)

In this case (no pun intended), I'd prefer keeping it as-is.

The Dart documentation shows this concept in the patterns overview:

…This allows you to simplify the variable pattern from something redundant like key: key to just :key

(and it goes into a bit more detail in the pattern types page)

nate-thegrate · 2024-07-01T20:55:22Z

doesn't help that : and final have the same color highlight

Yes, I can identify with that. Same thing goes with enum values: they have a distinct color in VS Code, but on GitHub they look just like other static class members.

nate-thegrate · 2024-07-01T23:35:56Z

Thank you Victor!

I think we're good to merge, since test updates are included in this PR.
(My plan was to ping @test-exemption-reviewer anyway, but it looks like they're pretty backed up as of now.)

I really like how patterns can be used for variable assignment and avoiding duplicated logic. (related: flutter#150942) ```dart // before final GestureRecognizer? recognizer = info.recognizer; if (recognizer is TapGestureRecognizer) { if (recognizer.onTap != null) { configuration.onTap = recognizer.onTap; configuration.isLink = true; } } else if (recognizer is DoubleTapGestureRecognizer) { if (recognizer.onDoubleTap != null) { configuration.onTap = recognizer.onDoubleTap; configuration.isLink = true; } } // after switch (info.recognizer) { case TapGestureRecognizer(:final VoidCallback? onTap): case DoubleTapGestureRecognizer(onDoubleTap: final VoidCallback? onTap): if (onTap != null) { configuration.onTap = onTap; configuration.isLink = true; } } ```

flutter/flutter@99bb2ff...af913a7 2024-07-02 [email protected] Use `ErrorHandlingFileSystem.deleteIfExists` when deleting .plugin_symlinks (flutter/flutter#151073) 2024-07-02 [email protected] ScrollEndNotification example: auto-scroll based on RenderSliver constraints and geometry (flutter/flutter#143538) 2024-07-02 [email protected] Roll Packages from 412ec46 to d2705fb (13 revisions) (flutter/flutter#151169) 2024-07-02 [email protected] docimports for painting (flutter/flutter#151143) 2024-07-02 [email protected] docimports for scheduler (flutter/flutter#151126) 2024-07-02 [email protected] `dismissible.dart` code cleanup (flutter/flutter#150276) 2024-07-02 [email protected] docimports for physics (flutter/flutter#151125) 2024-07-02 [email protected] docimports for services (flutter/flutter#151134) 2024-07-02 [email protected] docimports for cupertino (flutter/flutter#151149) 2024-07-02 [email protected] docimports for gestures (flutter/flutter#151123) 2024-07-02 [email protected] Docimports for foundation (flutter/flutter#151119) 2024-07-02 [email protected] docimports for semantics (flutter/flutter#151132) 2024-07-02 [email protected] [flutter_driver] add allocator mtl to memory event allowlist. (flutter/flutter#151153) 2024-07-02 [email protected] Roll Flutter Engine from 40c087b31515 to 433d360eee11 (7 revisions) (flutter/flutter#151165) 2024-07-02 [email protected] Refactor BuildInfo to always require packageConfigPath (flutter/flutter#150559) 2024-07-02 [email protected] Roll Flutter Engine from d3c5bd66a78f to 40c087b31515 (1 revision) (flutter/flutter#151156) 2024-07-02 [email protected] Roll Flutter Engine from fc5bc14e6091 to d3c5bd66a78f (1 revision) (flutter/flutter#151155) 2024-07-02 [email protected] Fix: `CupertinoActionSheet` should take up max height when actions section is short (flutter/flutter#150708) 2024-07-02 [email protected] Roll Flutter Engine from 3456fee1a6b9 to fc5bc14e6091 (8 revisions) (flutter/flutter#151150) 2024-07-02 [email protected] [tool] remove some temporary printTrace calls (flutter/flutter#151074) 2024-07-01 [email protected] Implementing a few switch statements (flutter/flutter#150946) 2024-07-01 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Upgrade template Gradle, App AGP, Module AGP, and Kotlin versions, and tests (#150969)" (flutter/flutter#151147) 2024-07-01 [email protected] Upgrade template Gradle, App AGP, Module AGP, and Kotlin versions, and tests (flutter/flutter#150969) 2024-07-01 [email protected] Roll Flutter Engine from b57a044ed10f to 3456fee1a6b9 (5 revisions) (flutter/flutter#151127) 2024-07-01 [email protected] Read AndroidManifest.xml and emit manifest-aar-impeller-(enabled|disabled) analytics (flutter/flutter#150970) 2024-07-01 [email protected] More docimports for animation library (flutter/flutter#151011) 2024-07-01 [email protected] Bump dartdoc to 8.0.10 (flutter/flutter#151107) 2024-07-01 [email protected] Fix missing `[` in docs (flutter/flutter#151091) 2024-07-01 [email protected] Roll pub packages (flutter/flutter#151028) 2024-07-01 [email protected] Fix teardown of a FocusScopeNode in material/app_test (flutter/flutter#151115) 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 Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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

I really like how patterns can be used for variable assignment and avoiding duplicated logic. (related: flutter#150942) ```dart // before final GestureRecognizer? recognizer = info.recognizer; if (recognizer is TapGestureRecognizer) { if (recognizer.onTap != null) { configuration.onTap = recognizer.onTap; configuration.isLink = true; } } else if (recognizer is DoubleTapGestureRecognizer) { if (recognizer.onDoubleTap != null) { configuration.onTap = recognizer.onDoubleTap; configuration.isLink = true; } } // after switch (info.recognizer) { case TapGestureRecognizer(:final VoidCallback? onTap): case DoubleTapGestureRecognizer(onDoubleTap: final VoidCallback? onTap): if (onTap != null) { configuration.onTap = onTap; configuration.isLink = true; } } ```

nate-thegrate force-pushed the switch-statement-refactor branch from b746bef to bc2dcd7 Compare June 27, 2024 19:41

nate-thegrate marked this pull request as ready for review June 27, 2024 19:41

justinmc requested a review from victorsanni June 27, 2024 21:49

victorsanni reviewed Jul 1, 2024

View reviewed changes

nate-thegrate added 3 commits July 1, 2024 15:04

implement switch statements

8191763

minor cleanup

f4dc4d9

implement feedback from code review

f7100bf

nate-thegrate force-pushed the switch-statement-refactor branch from 4b73180 to f7100bf Compare July 1, 2024 21:05

victorsanni approved these changes Jul 1, 2024

View reviewed changes

nate-thegrate added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 1, 2024

auto-submit bot merged commit d07a165 into flutter:master Jul 1, 2024

nate-thegrate deleted the switch-statement-refactor branch July 1, 2024 23:36

engine-flutter-autoroll mentioned this pull request Jul 2, 2024

Roll Flutter from 99bb2ff6a614 to a052fb763814 (17 revisions) flutter/packages#7041

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 2, 2024

d07a165 Implementing a few switch statements (flutter/flutter#150946)

55490ff

engine-flutter-autoroll mentioned this pull request Jul 2, 2024

Roll Flutter from 99bb2ff6a614 to 61326afdae44 (18 revisions) flutter/packages#7042

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 2, 2024

d07a165 Implementing a few switch statements (flutter/flutter#150946)

5e76288

engine-flutter-autoroll mentioned this pull request Jul 2, 2024

Roll Flutter from 99bb2ff6a614 to 455be19d928f (20 revisions) flutter/packages#7043

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 2, 2024

d07a165 Implementing a few switch statements (flutter/flutter#150946)

809c2b0

engine-flutter-autoroll mentioned this pull request Jul 2, 2024

Roll Flutter from 99bb2ff6a614 to 12e3635e8e27 (27 revisions) flutter/packages#7045

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 2, 2024

d07a165 Implementing a few switch statements (flutter/flutter#150946)

90ebd45

engine-flutter-autoroll mentioned this pull request Jul 2, 2024

Roll Flutter from 99bb2ff6a614 to 9527259c705c (28 revisions) flutter/packages#7046

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 2, 2024

d07a165 Implementing a few switch statements (flutter/flutter#150946)

307e7fd

engine-flutter-autoroll mentioned this pull request Jul 2, 2024

Roll Flutter from 99bb2ff6a614 to af913a75b5d2 (30 revisions) flutter/packages#7047

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 2, 2024

d07a165 Implementing a few switch statements (flutter/flutter#150946)

badbe81

nate-thegrate added the refactor Improving readability/efficiency without behavioral changes label Jul 12, 2024

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024

d07a165 Implementing a few switch statements (flutter/flutter#150946)

4ddacf3

Implementing a few switch statements #150946

Implementing a few switch statements #150946

Uh oh!

Conversation

nate-thegrate commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

victorsanni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

victorsanni commented Jul 1, 2024

Uh oh!

victorsanni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nate-thegrate commented Jul 1, 2024

Uh oh!

nate-thegrate commented Jul 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nate-thegrate commented Jun 27, 2024 •

edited

Loading