Skip to content

Conversation

@nate-thegrate
Copy link
Contributor

This PR is step 12 in the journey to solve issue #136139 and make the entire Flutter repo more readable.

Most of it involves implementing switch expressions, and there's also a few other random things that I wanted to clean up a bit.

@github-actions github-actions bot added a: tests "flutter test", flutter_test, or one of our tests a: text input Entering text in a text field or keyboard related problems tool Affects the "flutter" command-line tool. See also t: labels. framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. a: internationalization Supporting other languages or locales. (aka i18n) f: scrolling Viewports, list views, slivers, etc. f: cupertino flutter/packages/flutter/cupertino repository f: integration_test The flutter/packages/integration_test plugin labels May 15, 2024
@nate-thegrate nate-thegrate marked this pull request as ready for review May 15, 2024 07:47
@Piinks Piinks requested a review from victorsanni May 15, 2024 18:22
Copy link
Contributor

@victorsanni victorsanni left a comment

Choose a reason for hiding this comment

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

Hi @nate-thegrate! I've left a couple comments, and once addressed I will go ahead and approve. Thanks for the work!

}
return null;
return switch (this) {
study => null,
Copy link
Contributor

Choose a reason for hiding this comment

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

I am confused as to how GalleryDemoCategory.study becomes just study (same for the other values of GalleryDemoCategory. I can't find where these variables were assigned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you're inside a class declaration, you can use its static members without needing to type out the class name:

class MyClass {
  static const value = 42;

  void foo() {
    print(MyClass.value); // you can use the class name, but it's not necessary

    print(value); // it works!
  }
}

void foo() {
  print(MyClass.value); // name is required outside the class
}

An enum declaration works the same way:

enum MyEnum {
  a,
  b,
  c;

  void foo() {
    print('$a $b $c');
  }
}

void foo() {
  print('${MyEnum.a} ${MyEnum.b} ${MyEnum.c}');
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I did not know this was possible. Thanks for enlightening me.

}
throw SerializationException(
'Unsupported wait condition $conditionName in ${waitCondition.serialize()}');
return switch (conditionName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the variable conditionName still necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope! I just have it here because of the line length guideline:

WaitCondition deserializeCondition(SerializableWaitCondition waitCondition) {
  return switch (waitCondition.conditionName) {
    // ...
    final String conditionName => throw SerializationException('Unsupported wait condition $conditionName in ${waitCondition.serialize()}'),
  };

Defining the variable beforehand lets us use a _ instead.

@victorsanni victorsanni added the autosubmit Merge PR when tree becomes green via auto submit App label May 16, 2024
Copy link
Contributor Author

@nate-thegrate nate-thegrate left a comment

Choose a reason for hiding this comment

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

+301 -529 is a lot of diffs to go through—thanks for the review and for your attention to detail!

}
return null;
return switch (this) {
study => null,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you're inside a class declaration, you can use its static members without needing to type out the class name:

class MyClass {
  static const value = 42;

  void foo() {
    print(MyClass.value); // you can use the class name, but it's not necessary

    print(value); // it works!
  }
}

void foo() {
  print(MyClass.value); // name is required outside the class
}

An enum declaration works the same way:

enum MyEnum {
  a,
  b,
  c;

  void foo() {
    print('$a $b $c');
  }
}

void foo() {
  print('${MyEnum.a} ${MyEnum.b} ${MyEnum.c}');
}

}
throw SerializationException(
'Unsupported wait condition $conditionName in ${waitCondition.serialize()}');
return switch (conditionName) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope! I just have it here because of the line length guideline:

WaitCondition deserializeCondition(SerializableWaitCondition waitCondition) {
  return switch (waitCondition.conditionName) {
    // ...
    final String conditionName => throw SerializationException('Unsupported wait condition $conditionName in ${waitCondition.serialize()}'),
  };

Defining the variable beforehand lets us use a _ instead.

@victorsanni victorsanni merged commit fa9992e into flutter:master May 17, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 17, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 17, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request May 17, 2024
flutter/flutter@0d22d91...00425ef

2024-05-17 [email protected] Roll Flutter Engine from d575e511f9e5 to a19d3722922d (2 revisions) (flutter/flutter#148534)
2024-05-17 [email protected] Roll Flutter Engine from 8d1a1d8d7b48 to d575e511f9e5 (5 revisions) (flutter/flutter#148529)
2024-05-17 [email protected] Manual roll Flutter Engine from 6fa734d68688 to 8d1a1d8d7b48 (8 revisions) (flutter/flutter#148528)
2024-05-17 [email protected] Add test for material_banner.0.dart and material_banner.1.dart (flutter/flutter#148452)
2024-05-17 [email protected] `switch` statement cleanup (flutter/flutter#148382)
2024-05-16 [email protected] const vs. non-const widget build benchmark (flutter/flutter#148261)
2024-05-16 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 4.1.5 to 4.1.6 (flutter/flutter#148516)
2024-05-16 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from 6fa734d68688 to 1850def2ccff (1 revision) (#148507)" (flutter/flutter#148517)
2024-05-16 [email protected] Add PrivacyInfo.xcprivacy to plugin template (flutter/flutter#148485)
2024-05-16 [email protected] Fix iOS reference in macOS Cocoapods error (flutter/flutter#148506)
2024-05-16 [email protected] Roll Flutter Engine from 6fa734d68688 to 1850def2ccff (1 revision) (flutter/flutter#148507)
2024-05-16 [email protected] Roll Flutter Engine from 460df6caef0e to 6fa734d68688 (4 revisions) (flutter/flutter#148500)
2024-05-16 [email protected] Enhanced enum features for `AnimationStatus` (flutter/flutter#147801)
2024-05-16 [email protected] [macOS] codesign native assets during embed (flutter/flutter#148310)

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] 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
TecHaxter pushed a commit to TecHaxter/flutter_packages that referenced this pull request May 22, 2024
flutter/flutter@0d22d91...00425ef

2024-05-17 [email protected] Roll Flutter Engine from d575e511f9e5 to a19d3722922d (2 revisions) (flutter/flutter#148534)
2024-05-17 [email protected] Roll Flutter Engine from 8d1a1d8d7b48 to d575e511f9e5 (5 revisions) (flutter/flutter#148529)
2024-05-17 [email protected] Manual roll Flutter Engine from 6fa734d68688 to 8d1a1d8d7b48 (8 revisions) (flutter/flutter#148528)
2024-05-17 [email protected] Add test for material_banner.0.dart and material_banner.1.dart (flutter/flutter#148452)
2024-05-17 [email protected] `switch` statement cleanup (flutter/flutter#148382)
2024-05-16 [email protected] const vs. non-const widget build benchmark (flutter/flutter#148261)
2024-05-16 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 4.1.5 to 4.1.6 (flutter/flutter#148516)
2024-05-16 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from 6fa734d68688 to 1850def2ccff (1 revision) (#148507)" (flutter/flutter#148517)
2024-05-16 [email protected] Add PrivacyInfo.xcprivacy to plugin template (flutter/flutter#148485)
2024-05-16 [email protected] Fix iOS reference in macOS Cocoapods error (flutter/flutter#148506)
2024-05-16 [email protected] Roll Flutter Engine from 6fa734d68688 to 1850def2ccff (1 revision) (flutter/flutter#148507)
2024-05-16 [email protected] Roll Flutter Engine from 460df6caef0e to 6fa734d68688 (4 revisions) (flutter/flutter#148500)
2024-05-16 [email protected] Enhanced enum features for `AnimationStatus` (flutter/flutter#147801)
2024-05-16 [email protected] [macOS] codesign native assets during embed (flutter/flutter#148310)

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] 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
@nate-thegrate 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: internationalization Supporting other languages or locales. (aka i18n) a: tests "flutter test", flutter_test, or one of our tests a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App f: cupertino flutter/packages/flutter/cupertino repository f: integration_test The flutter/packages/integration_test plugin f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. refactor Improving readability/efficiency without behavioral changes tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants