Reorder the checkboxes in the PR template #3666
Conversation
...so that they are listed in more or less the likely order that people will do them in.
There was a problem hiding this comment.
LGTM. Some optional thoughts.
(I'm curious whether you've found the checklist to actually be effective as a checklist; my experience has been that people mostly just check all the boxes whether they've done them or not. E.g., my impression from triage in this repo is that most PRs that don't list an issue have the box checked for listing an issue.)
| - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. | ||
| - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. | ||
| - [ ] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. | ||
| - [ ] I signed the [CLA]. |
There was a problem hiding this comment.
New contributors generally do this after creating the PR since that's when the bot tells them to. I'm not sure whether the ordering is intended to be new or established contributors; I would have thought the former since for established contributors the checklist is much more rote.
There was a problem hiding this comment.
Well to be frank I put the order in the order that I usually complete them, but in any case, wouldn't the order be the same here for new or established contributors? New ones should hopefully read the contributor guide first, then the other two, before they write their PR, and certainly before they get it reviewed, but they don't need to sign the CLA until it's ready to land, really. And established contributors have already done all four.
| - [ ] I listed at least one issue that this PR fixes in the description above. | ||
| - [ ] I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt. | ||
| - [ ] I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy]. | ||
| - [ ] I updated CHANGELOG.md to add a description of the change. |
There was a problem hiding this comment.
I would probably put these two last.
There was a problem hiding this comment.
Well until you update the changelog, the tests can't pass, so I figured the tests one had to go last, and then for thematic consistency I put the other test one with it...
There was a problem hiding this comment.
There's only a failure from not updating the CHANGELOG of you update the version, so as long as these two stay together it won't affect tests.
There was a problem hiding this comment.
Surely the checkbox for "all the tests pass" has to go last though? Since as soon as you make any change the tests have to run again, so you can't know if the tests have passed yet? Seems like by definition you have to check the "changelog" one before the "tests" one...
Anyway I don't feel strongly about this at all, I'm happy to defer to whatever order you'd like to see. Let me know what order you want it in and I'll land that. :-)
There was a problem hiding this comment.
I guess that makes sense. LGTM as-is.
I would take that as a sign that the person isn't reading our contributing materials and treat the PR accordingly (e.g. close it with a polite comment saying that it seems the instructions were not followed). I originally found these templates annoying but after the recent edits to make them more streamlined I've ended up finding I do use them, they're a useful way to make sure I remembered everything especially in repos I'm not frequently contributing to. |
| - [ ] I listed at least one issue that this PR fixes in the description above. | ||
| - [ ] I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt. | ||
| - [ ] I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy]. | ||
| - [ ] I updated CHANGELOG.md to add a description of the change. |
There was a problem hiding this comment.
I guess that makes sense. LGTM as-is.
* master: (49 commits) Prep for alignment with Flutter analysis options (flutter#3703) [google_maps_flutter_platform_interface] Mark constructors as const for ids (flutter#3718) [image_picker] Endorse image_picker_for_web (flutter#3717) Add missing licenses, and add a check (flutter#3720) [ci] Add libgcrypt to Docker image. (flutter#3711) Reorder the checkboxes in the PR template (flutter#3666) Re-endorse connectivity_for_web (flutter#3708) Fix missing declaration of windows' default_package (flutter#3705) Typos in comments (flutter#2846) Skip flutter upgrade for pod linting Cirrus task (flutter#3700) [cross_file] Delete. (flutter#3698) [tool] Improve check version ci so that it enforces the version in CHANGELOG and pubspec matches. (flutter#3678) Streamline CI setup, and reenable macOS credits (flutter#3697) [video_player] fixed misleading size and aspect ratio documentation (flutter#3668) [image_picker] Implemented 2860 and added Unit Test to test functionality (flutter#3685) [shared_preferences] Fix concurrent modification of the shared preferences on Android (flutter#3684) [extension_google_sign_in_as_googleapis_auth] Deleted. (flutter#3694) Skip pod lint tests (flutter#3692) [Video_Player] Remove the deprecated API reference. (flutter#3669) [google_sign_in] fix test(flutter#3690) ...
...so that they are listed in more or less the likely order that people will do them in.
Pre-launch Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.