Add advice on code review for this repo #4698
Conversation
There was a problem hiding this comment.
I think this is fine. Waiting for @stuartmorgan is probably still a decent idea though.
CONTRIBUTING.md
Outdated
|
|
||
| Each package has a code owner (specified in | ||
| [CODEOWNERS](https://github.com/flutter/packages/blob/main/CODEOWNERS)); | ||
| PRs will automatically be assigned to the appropriate code owners for review. |
There was a problem hiding this comment.
Why do we need to say this? People contributing PRs don't generally need to know how reviews are assigned, only that it happens (which it will).
There was a problem hiding this comment.
We don't have to, it's just nice for people like me to know how it's done. I mean, it's not a secret right? Is there harm in documenting it?
There was a problem hiding this comment.
The only harm is my concern about the overall length of the combined sum of all of our contributing docs. Each individual line seems harmless, but it's death by a thousand cuts.
CONTRIBUTING.md
Outdated
| Each package has a code owner (specified in | ||
| [CODEOWNERS](https://github.com/flutter/packages/blob/main/CODEOWNERS)); | ||
| PRs will automatically be assigned to the appropriate code owners for review. | ||
| Review is expected within a week or two (but preferred much quicker than that!). |
There was a problem hiding this comment.
This is redundant with https://github.com/flutter/flutter/wiki/Tree-hygiene (including the tl;dr at the top) which is part of the contributing guide we already point to. I would prefer to avoid redundant docs, especially given that I think the biggest problem our contributing docs have is that they are incredibly long.
There was a problem hiding this comment.
I mean, all I really need here is lines 28-31 but it will look mighty silly if that's the only text here.
There was a problem hiding this comment.
How about we collapse these first two paragraphs to just:
PRs will automatically be assigned to [code owners](https://github.com/flutter/packages/blob/main/CODEOWNERS) for review. If a code owner is creating a PR, they should explicitly pick a
[Flutter team member](https://github.com/flutter/flutter/wiki/Contributor-access)
as a code reviewer.
flutter/packages@9bf208f...3b602e7 2023-08-16 [email protected] Add advice on code review for this repo (flutter/packages#4698) 2023-08-16 [email protected] [platform] Import the `platform` package (flutter/packages#4613) 2023-08-16 [email protected] Roll Flutter from f0e7c51 to 2502b51 (15 revisions) (flutter/packages#4722) 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-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).