Adaptive Scaffold Widget Package Boilerplating #2377
Conversation
youssef-attia
changed the title
youssef-attia
changed the title
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
|
||
| library adaptive_scaffold; | ||
|
|
||
| /// A Calculator. |
There was a problem hiding this comment.
Why are we checking in a package with no actual functionality? We should wait until we have at least some useful, real content before landing a package (and especially before publishing a package, which this PR will do currently.)
There was a problem hiding this comment.
Because of the size of the PR. It was recommended to split it into 3 PRs to make it easier to review. This PR would just hold boilerplate, the next would hold code/tests, and the last would hold the examples. I expect the PRs to be sent out in succession so it would be fully out before anyone could use it in some stable version. Is this not a good way to handle it?
There was a problem hiding this comment.
If it's going to be landed without any functionality, it should have publish_to: none in the pubspec until it's actually a useful package.
This seems like an odd cut-point to me though; there's almost nothing in this PR that requires actual review since it's almost all boilerplate, so I'm not sure how including initial functionality would make reviewing it harder. But if @dkwingsmt feels strongly about landing this now, it needs to not be published.
There was a problem hiding this comment.
I think just having the boilerplate at first makes it easier to drop the perceived changes significantly and makes the review process simpler for everyone involved. I've added the publish_to:none flag but if there's anything else you'd like me to do to make this more streamlined please let me know. Thanks.
|
Also, please update the PR description to describe the PR, not the project. It's fine to link to the design document for context (although there should also be an issue tracking this, ideally, which you would link to for easier tracking), but the primary description should be of what is in this PR, not what the whole project will be. |
youssef-attia
changed the title
|
Moved to #2406. |
The initial boilerplate for the AdaptiveScaffold suite. First stage in a multistage release. The next stages will introduce actual functionality and examples.
#108827
Described further in the design doc