-
Notifications
You must be signed in to change notification settings - Fork 29.7k
pattern-matching refactor #154753
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.
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
pattern-matching refactor #154753
Conversation
1f3053b to
7470d6f
Compare
|
I took a cursory glance (it's EOD Friday here so I'll have to look again next week) and in general this seems like an improvement, and in the couple of places where code owners might prefer the legacy style I suspect you could make 1-off adjustments. That being said, I'm having a hard time understanding the state of the proposal; the original design has a combination of positive and negative comments, and the only other (public?) comment I see from @goderbauer implies not a strong amount of support. If there were other outcomes or conversations you can share to explain this change, that would be great. |
|
Sure! I'd be happy to give an overview of what I've learned. Hixie engaged with the doc the most, and also was the most strongly opposed to pattern-matching out of the people who responded. His negative responses (e.g. here, here, here, and here) are summarized really well by a comment he made at the top: simplicity is more important than concision. There were other spots where he gave a more neutral response (here, here, here, and here) along the lines of "neither one is very readable." As far as his supportive feedback, one comment in particular stuck out:
Mylo Fawcett also engaged quite a bit and showed more support overall for implementing patterns. I was also interested to hear from Bob Nystrom that Dart pattern-matching has a lot in common with Swift syntax. It's definitely not cut-and-dry whether pattern-matching is overall "better" or "worse" than the alternative (there are some situations where whether the pattern is more readable depends on how long the variable names are). There's a lot of variety, both with regards to how patterns would apply to a particular code snippet, and also pertaining to the individual reactions and overall opinions that different people have. That being said, there was one huge takeaway that hadn't occurred to me until I started talking with people and gathering feedback: map patterns are far more readable than object patterns. if (someMap case {'key': final Object value}) {
// ...
}It's distinct, concise, and expressive: the By comparison, object patterns are often not as great, especially with the leading colon:
To decide what went into this PR, I basically just included all the map patterns, and then some other things that I thought looked nice.
Sounds like a great plan! |
7470d6f to
54ad329
Compare
54ad329 to
e2cbb9c
Compare
victorsanni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love how this reduces the if foo is null/T checks. Also love the switch statements, especially when they're aligned nicely. The if..case..when statements also make more sense to me now.
dev/bots/test/analyze-gen-defaults/dev/tools/gen_defaults/bin/template.dart
Outdated
Show resolved
Hide resolved
ebcf9c7 to
a66890e
Compare
Piinks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, some of the case statements gave me the heebie-jeebies initially but once I cam across something I knew really well, they made sense.
Regarding anonymous parameters, maybe we need to revisit that in the style guide, as there are a lot of them here. I wonder if they're already pervasive in the codebase and the style guide is just not reflecting reality...
I'll circulate this with the team once more so we can finally move this forward.
I always love your creative titles, but can you update it to reflect the change now? Thanks!
Manual roll requested by [email protected] flutter/flutter@ec2e12b...0917e9d 2024-10-05 [email protected] Roll Flutter Engine from 2068577e510b to 01de9dcf6bbf (1 revision) (flutter/flutter#156274) 2024-10-05 [email protected] Roll Flutter Engine from 6d6bc39fe8b1 to 2068577e510b (1 revision) (flutter/flutter#156267) 2024-10-05 [email protected] Roll Flutter Engine from 92b5b318190b to 6d6bc39fe8b1 (4 revisions) (flutter/flutter#156266) 2024-10-05 [email protected] Roll Flutter Engine from fd6082ab6693 to 92b5b318190b (1 revision) (flutter/flutter#156261) 2024-10-05 [email protected] Roll Flutter Engine from bc5f9fc80b06 to fd6082ab6693 (4 revisions) (flutter/flutter#156258) 2024-10-04 [email protected] Roll Flutter Engine from eece6c3ec63a to bc5f9fc80b06 (2 revisions) (flutter/flutter#156252) 2024-10-04 [email protected] Roll pub packages (flutter/flutter#156254) 2024-10-04 [email protected] Roll Flutter Engine from 9fa363bdde20 to eece6c3ec63a (1 revision) (flutter/flutter#156250) 2024-10-04 [email protected] Roll Flutter Engine from d38f5e560a98 to 9fa363bdde20 (1 revision) (flutter/flutter#156249) 2024-10-04 [email protected] remove bringup from Windows tool_integration_tests_* (flutter/flutter#156179) 2024-10-04 [email protected] Roll Flutter Engine from 205484009711 to d38f5e560a98 (4 revisions) (flutter/flutter#156239) 2024-10-04 [email protected] Normalize `ThemeData.cardTheme` (flutter/flutter#153254) 2024-10-04 [email protected] Roll Packages from 429650f to 05bf1d4 (5 revisions) (flutter/flutter#156225) 2024-10-04 [email protected] Roll Flutter Engine from e0109f0fd5b2 to 205484009711 (1 revision) (flutter/flutter#156216) 2024-10-04 [email protected] Roll pub packages (flutter/flutter#156215) 2024-10-04 [email protected] Roll Flutter Engine from c2a9fb051bd0 to e0109f0fd5b2 (1 revision) (flutter/flutter#156213) 2024-10-04 [email protected] Roll Flutter Engine from 66d397dff87a to c2a9fb051bd0 (5 revisions) (flutter/flutter#156206) 2024-10-04 [email protected] Roll Flutter Engine from 20369c5d2b93 to 66d397dff87a (5 revisions) (flutter/flutter#156188) 2024-10-03 [email protected] Roll Flutter Engine from de1762dbc5cc to 20369c5d2b93 (7 revisions) (flutter/flutter#156174) 2024-10-03 [email protected] Refactor devfs_web_ddc_modules_test.dart (flutter/flutter#155609) 2024-10-03 [email protected] Allow arrow keys to navigate `MenuAnchor` independently of global shortcut definition (flutter/flutter#155728) 2024-10-03 [email protected] Inherited Theme: zero rebuilds (flutter/flutter#155699) 2024-10-03 [email protected] Roll Flutter Engine from 247bc68c578e to de1762dbc5cc (4 revisions) (flutter/flutter#156160) 2024-10-03 [email protected] Roll pub packages (flutter/flutter#156159) 2024-10-03 [email protected] pattern-matching refactor (flutter/flutter#154753) 2024-10-03 [email protected] Add autocorrect and enableSuggestions to SearchDelegate (flutter/flutter#154932) 2024-10-03 [email protected] Roll Packages from 7c97c88 to 429650f (6 revisions) (flutter/flutter#156155) 2024-10-03 [email protected] Move some tools test ownership to Ben (flutter/flutter#156123) 2024-10-03 [email protected] further shard the Windows tool_integration_tests* targets (flutter/flutter#156121) 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
This egotistical PR aims to draw attention to a [style guideline](https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#use--for-getters-and-callbacks-that-just-return-literals-or-switch-expressions) that I changed: > #### Use `=>` for getters and callbacks that just return literals or switch expressions <br> 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.
This pull request aims to improve code readability, based on feedback gathered in a recent design doc.
There are two factors that hugely impact how easy it is to understand a piece of code: verbosity and complexity.
Reducing verbosity is important, because boilerplate makes a project more difficult to navigate. It also has a tendency to make one's eyes gloss over, and subtle typos/bugs become more likely to slip through.
Reducing complexity makes the code more accessible to more people. This is especially important for open-source projects like Flutter, where the code is read by those who make contributions, as well as others who read through source code as they debug their own projects.
The following examples show how pattern-matching might affect these two factors:
Example 1 (GOOD)
[click to expand]Without using patterns, this might expand to
Had
ancestorbeen a non-local variable, it would need to be "converted" as well:Example 2 (BAD)
[click to expand]Assuming
widgetis a non-local variable, this would expand to:In both of the examples above, an
if-casestatement simultaneously verifies that an object meets the specified criteria and performs a variable assignment accordingly.But there are some differences: Example 2 uses a more deeply-nested pattern than Example 1 but makes fewer useful checks.
Example 1:
ancestoris anInheritedElementwidgetis anInheritedThemeExample 2:
widgetis aPreferredSizeWidget(every
PreferredSizeWidgethas asizefield, and everySizehas aheightfield)I feel hesitant to try presenting a set of cut-and-dry rules as to which scenarios should/shouldn't use pattern-matching, since there are an abundance of different types of patterns, and an abundance of different places where they might be used.
But hopefully the conversations we've had recently will help us converge toward a common intuition of how pattern-matching can best be utilized for improved readability.