Skip to content

Implementing switch expressions throughout the repo. #136140

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.

Sign up for GitHub

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

Jump to bottom
Closed
nate-thegrate wants to merge 28 commits into from
Closed

Implementing switch expressions throughout the repo. #136140

nate-thegrate wants to merge 28 commits into from

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