Add "Command Pattern" and "Result Class" design patterns #11444
Conversation
miquelbeltran
requested review from
sfshaza2,
parlough and
a team
as code owners
|
Visit the preview URL for this PR (updated for commit 3d54a39): https://flutter-docs-prod--pr11444-mb-result-command-nk001jlx.web.app |
There was a problem hiding this comment.
I'm trying to be more helpful in reviewing the text. I'm mostly confident in my suggestions, and I think it's okay to commit them before Shams or Parker reviews, because it might just make their reviews harder if they see my comments. But they might ask you to change things I suggested 😄
Co-authored-by: Eric Windmill <[email protected]>
Co-authored-by: Eric Windmill <[email protected]>
|
Just wondering if we should really be recommending random packages here especially when:
While I understand that it's nice to point out existing solutions in the community when it makes sense I feel like this should still meet certain criteria that this in fact may do but it's just very unclear what that criteria is or if this represents an endorsement or simply a random piece of information. |
|
Thanks for the heads up @bivens-dev I did not look deep into it, but I looked at the sample and repo activity and it seemed fine. It's true that the package listing seems broken (they have a License in the repo, but it's not correctly listed in pub). Not having a Flutter Favorite shouldn't be a blocker imo. I am ok if @ericwindmill thinks it is better to not mention it, and just mention that other packages exists on pub.dev. |
|
If we decide to go with "Check pub.dev for different ready-to-use implementations of the command pattern." We should do the same for the Result class. |
|
I agree that we should be diligent in suggesting packages, but in this case I feel comfortable suggesting it because the author is a trusted member of the community. |
There was a problem hiding this comment.
Thanks for working on this @miquelbeltran and addressing all of Eric's and Sham's feedback!
I haven't read through the cookbook recipes yet, but I wanted to dive into the two full classes in question. Both their structure and their doc comments, so that they can be self documenting but also to ensure we encourage best practices. I hope we can set them up to be great defaults, as they will surely be copy pasted.
Let me know what you think and please do push back on anything that doesn't make sense or is out of scope. Thanks again :D
\cc @ericwindmill
Co-authored-by: Parker Lougheed <[email protected]>
…ors (#2538) From my review of the recipes PR in flutter/website#11444 (review).
As discussed in the PR for the Result pattern implementation (flutter/website#11444) @parlough recommended that `asError` and `asOk` should be not be used, and instead we should use proper exhaustiveness checking. This PR removes the two "convenience" methods and refactors code. In some cases, it was enough with writing a proper `if` clause, while in others it was necessary to use a `switch`. Still, they are present in the `testing` folder, as they can be useful for testing purposes. ## Pre-launch Checklist - [x] I read the [Flutter Style Guide] _recently_, and have followed its advice. - [x] I signed the [CLA]. - [x] I read the [Contributors Guide]. - [x] I have added sample code updates to the [changelog]. - [x] I updated/added relevant documentation (doc comments with `///`). If you need help, consider asking for advice on the #hackers-devrel channel on [Discord]. <!-- Links --> [Flutter Style Guide]: https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md [CLA]: https://cla.developers.google.com/ [Discord]: https://github.com/flutter/flutter/blob/master/docs/contributing/Chat.md [Contributors Guide]: https://github.com/flutter/samples/blob/main/CONTRIBUTING.md [changelog]: ../CHANGELOG.md
There was a problem hiding this comment.
Thanks for making all of those adjustments @miquelbeltran! This is looking great.
I didn't complete a full review of the remaining text as Shams and Eric seemed to do so, but left some minor comments and considerations.
Approving so you or @ericwindmill can land when you're ready. Thanks again!
|
|
||
| In the `Error<UserProfile>` case, | ||
| obtain the error object using the `error` property. | ||
|
|
There was a problem hiding this comment.
Not necessary for this initial PR, but do consider adding an example that uses an if case statement.
Co-authored-by: Parker Lougheed <[email protected]>
|
Thank you, @parlough ! I've applied your suggestions. I'll ensure there are no failing checks, and then @ericwindmill can go ahead with the merge. |
_Description of what this PR is changing or adding, and why:_ This PR adds an 'index' page for the Design Pattern cookbook recipes under App Architecture in the side nav. * Adds the page * Adds a `design-pattern.yml` file with * Adds an "Expansion Panel List" component (html + css) **This is still very much a work in progress.** Before merging: - [x] Land #11444 + add them to the data file, and create icons for their rows - [x] Update the `design-pattern.yml` with real data - [x] Create the remaining needing icons (right now all four list items use the same icon) - [x] Replace 'LOREM IPSUM, BROTHER' 😅 _Issues fixed by this PR (if any):_ * This will resolve #11374 --------- Co-authored-by: Parker Lougheed <[email protected]>
Description of what this PR is changing or adding, and why:
This PR introduces the "Command Pattern" and "Result Class" design pattern cookbook recipes.
Drafts were reviewed by @ericwindmill already
Issues fixed by this PR (if any):
Part of #11374
PRs or commits this PR depends on (if any):
Presubmit checklist