Skip to content

Conversation

@nate-thegrate
Copy link
Contributor

This egotistical PR aims to draw attention to a style guideline that I changed:

Use => for getters and callbacks that just return literals or switch expressions


There was an opportunity for valuable discussion in #154753 about how this structure affects readability, but I shut it down pretty quick since there was a lot of other stuff going on there.

Interested to hear thoughts from @Piinks and others.

@nate-thegrate nate-thegrate requested a review from Piinks October 9, 2024 16:44
@github-actions github-actions bot added a: tests "flutter test", flutter_test, or one of our tests tool Affects the "flutter" command-line tool. See also t: labels. framework flutter/packages/flutter repository. See also f: labels. a: animation Animation APIs f: material design flutter/packages/flutter/material repository. 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. a: desktop Running on desktop labels Oct 9, 2024
return false;
}
}
bool get _isOnDesktopAndWeb => kIsWeb || switch (defaultTargetPlatform) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks incorrect based on the name.. should this be true only iff it is desktop && web?

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, I am wondering if the original logic was incorrect to begin with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely: either the logic should be changed to use && or the getter should be renamed to _isOnDesktopOrWeb.

I could change it now, though perhaps that's outside the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah definitely something to follow up on separately.

@github-actions github-actions bot removed a: tests "flutter test", flutter_test, or one of our tests f: routes Navigator, Router, and related APIs. labels Oct 17, 2024
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

LGTM

@Piinks
Copy link
Contributor

Piinks commented Oct 17, 2024

Rebasing should fix the failing check.

@nate-thegrate nate-thegrate added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 17, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 17, 2024
@auto-submit

This comment was marked as resolved.

@nate-thegrate nate-thegrate added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 17, 2024
@auto-submit auto-submit bot merged commit 6b1bc45 into flutter:master Oct 17, 2024
@nate-thegrate nate-thegrate deleted the arrow-syntax branch October 17, 2024 19:25
@kevmoo
Copy link
Contributor

kevmoo commented Oct 18, 2024

Yes, please! More of this!

@nate-thegrate nate-thegrate added the refactor Improving readability/efficiency without behavioral changes label Oct 18, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 12, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: animation Animation APIs a: desktop Running on desktop autosubmit Merge PR when tree becomes green via auto submit App d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: cupertino flutter/packages/flutter/cupertino repository 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.

4 participants