Skip to content

Implementing switch expressions throughout the repo.#136140

Closed
nate-thegrate wants to merge 28 commits intoflutter:masterfrom
nate-thegrate:switch-expressions
Closed

Implementing switch expressions throughout the repo.#136140
nate-thegrate wants to merge 28 commits intoflutter:masterfrom
nate-thegrate:switch-expressions

Conversation

@nate-thegrate
Copy link
Contributor

@nate-thegrate nate-thegrate commented Oct 7, 2023 •
edited
Loading

Example: button_theme.dart

before:

EdgeInsetsGeometry get padding {
  if (_padding != null) {
    return _padding;
  }
  switch (textTheme) {
    case ButtonTextTheme.normal:
    case ButtonTextTheme.accent:
      return const EdgeInsets.symmetric(horizontal: 16.0);
    case ButtonTextTheme.primary:
      return const EdgeInsets.symmetric(horizontal: 24.0);
  }
}

after:

EdgeInsetsGeometry get padding {
  return _padding ?? switch (textTheme) {
    ButtonTextTheme.normal || ButtonTextTheme.accent => const EdgeInsets.symmetric(horizontal: 16.0),
    ButtonTextTheme.primary => const EdgeInsets.symmetric(horizontal: 24.0),
  };
}

(solves issue #136139)


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 Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (N/A).
  • I updated the tests to use switch expressions as well.
  • All existing and new tests are passing.

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

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. a: animation Animation APIs f: material design flutter/packages/flutter/material repository. a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) f: scrolling Viewports, list views, slivers, etc. f: cupertino flutter/packages/flutter/cupertino repository d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: routes Navigator, Router, and related APIs. f: focus Focus traversal, gaining or losing focus labels Oct 7, 2023
@nate-thegrate nate-thegrate marked this pull request as draft October 7, 2023 20:42
@flutter-dashboard
Copy link

flutter-dashboard bot commented Oct 7, 2023

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

stuartmorgan-g
stuartmorgan-g reviewed Oct 7, 2023
View reviewed changes
mateusfccp
mateusfccp reviewed Oct 9, 2023
View reviewed changes
mateusfccp
mateusfccp reviewed Oct 9, 2023
View reviewed changes
@github-actions github-actions bot added a: tests "flutter test", flutter_test, or one of our tests platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. a: internationalization Supporting other languages or locales. (aka i18n) a: desktop Running on desktop f: integration_test The flutter/packages/integration_test plugin labels Oct 10, 2023
@nate-thegrate nate-thegrate marked this pull request as ready for review October 11, 2023 00:35
@nate-thegrate
Copy link
Contributor Author

nate-thegrate commented Oct 11, 2023

All checks have passed! 🙌

@stuartmorgan @mateusfccp would one of you be able to approve the PR?

@mateusfccp
Copy link
Contributor

mateusfccp commented Oct 11, 2023

All checks have passed! 🙌

@stuartmorgan @mateusfccp would one of you be able to approve the PR?

I'm only a regular contributor

mateusfccp
mateusfccp reviewed Oct 11, 2023
View reviewed changes
dev/bots/service_worker_test.dart
Comment on lines +110 to +117
ServiceWorkerTestType.blockedServiceWorkers => 'index_with_blocked_service_workers.html',
ServiceWorkerTestType.withFlutterJs => 'index_with_flutterjs.html',
ServiceWorkerTestType.withoutFlutterJs => 'index_without_flutterjs.html',
ServiceWorkerTestType.withFlutterJsShort => 'index_with_flutterjs_short.html',
ServiceWorkerTestType.withFlutterJsEntrypointLoadedEvent => 'index_with_flutterjs_entrypoint_loaded.html',
ServiceWorkerTestType.withFlutterJsTrustedTypesOn => 'index_with_flutterjs_el_tt_on.html',
ServiceWorkerTestType.withFlutterJsCustomServiceWorkerVersion => 'index_with_flutterjs_custom_sw_version.html',
ServiceWorkerTestType.generatedEntrypoint => 'generated_entrypoint.html',
Copy link
Contributor

@mateusfccp mateusfccp Oct 11, 2023

Choose a reason for hiding this comment

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

Is this kind of alignment desired? @stuartmorgan

@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented Oct 11, 2023

Please don't keep pinging me here; my comment was just a drive-by flagging of a style guide violation, not a review. This PR will go through the normal framework PR review process and be assigned to the right person (not me) to make a decision about making a significant style change to the framework code.

@christopherfujino
Copy link
Contributor

christopherfujino commented Oct 19, 2023

@nate-thegrate I think this change is too large to effectively review. I would recommend breaking it up into many smaller PRs (maybe 5 libraries per PR?).

@nate-thegrate nate-thegrate mentioned this pull request Nov 27, 2023
Merged
8 tasks
auto-submit bot pushed a commit that referenced this pull request Nov 28, 2023
@nate-thegrate
Implement switch expressions in dev/ (#139048)
3267fbc 
I previously made a PR (#136140) that used `switch` expressions to make some parts of the Flutter codebase easier to understand. It was assigned to the framework team, and @christopherfujino let me know that it was too large to effectively review and recommended breaking it up into smaller pull requests.

Here's a PR that only targets files in the `dev/` directory. Hopefully this will be easier to work with!

(solves issue #136139)
caseycrogers pushed a commit to caseycrogers/flutter that referenced this pull request Dec 29, 2023
@nate-thegrate @caseycrogers
Implement switch expressions in dev/ (flutter#139048)
8ed0bcc 
I previously made a PR (flutter#136140) that used `switch` expressions to make some parts of the Flutter codebase easier to understand. It was assigned to the framework team, and @christopherfujino let me know that it was too large to effectively review and recommended breaking it up into smaller pull requests.

Here's a PR that only targets files in the `dev/` directory. Hopefully this will be easier to work with!

(solves issue flutter#136139)
@nate-thegrate nate-thegrate deleted the switch-expressions branch January 26, 2024 00:32
@nate-thegrate nate-thegrate mentioned this pull request May 10, 2024
Merged
@nate-thegrate nate-thegrate mentioned this pull request Oct 14, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stuartmorgan-g stuartmorgan-g stuartmorgan-g left review comments

+1 more reviewer

@mateusfccp mateusfccp mateusfccp left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) a: animation Animation APIs a: desktop Running on desktop 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 d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: cupertino flutter/packages/flutter/cupertino repository f: focus Focus traversal, gaining or losing focus f: integration_test The flutter/packages/integration_test plugin f: material design flutter/packages/flutter/material repository. f: routes Navigator, Router, and related APIs. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nate-thegrate @mateusfccp @stuartmorgan-g @christopherfujino