Update PULL_REQUEST_TEMPLATE.md #3801

cyanglaz · 2021-04-08T21:28:08Z

ditman · 2021-04-08T21:33:34Z

.github/PULL_REQUEST_TEMPLATE.md

@@ -8,7 +8,7 @@

 - [ ] 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 read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. (Note that not like the flutter/flutter repo, we do use dart formatter in the flutter/plugins repo.)


[ ] I have formatted this PR with `dartfmt -w .`, read the [Flutter Style Guide] and followed the [C++, Objective-C, Java style guides].

Is there a plugins command that we can provide so users can quickly format all the source they might have touched in their PR? Something that guarantees that the format step will pass?

does it make sense to say dart run ./script/tool/lib/src/main.dart format this those all of those.

blasten · 2021-04-08T22:10:04Z

.github/PULL_REQUEST_TEMPLATE.md

@@ -8,7 +8,7 @@

 - [ ] 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]. (Note that not like the flutter/flutter repo, we do use dart formatter in the flutter/plugins repo.)
+- [ ] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. (Note that not like the flutter/flutter repo, we do use dart formatter in the flutter/plugins repo. Run `dart run ./script/tool/lib/src/main.dart format` under plugins/ to auto format all source code.)


not like the -> unlike the

This will only work if they have done a pub get there. I think we should put instructions in CONTRIBUTING.md, and then link directly to that part of it from here.

cyanglaz · 2021-04-14T18:11:55Z

@stuartmorgan @ditman @blasten I updated the contribution guide to reflect the new tooling process we use, and updated the template to point user there.

blasten · 2021-04-14T20:11:58Z

CONTRIBUTING.md

@@ -48,6 +48,14 @@ for any of the following plugins, we encourage you to submit it
   fetch from the master repository, not your clone, when running `git fetch`
   et al.)

+
+## Setting up tools
+


nit: extra line

blasten · 2021-04-14T20:12:14Z

CONTRIBUTING.md

+
+We use a range of tooling script to do varies of things on CI. (testing, formatting, etc.)
+Your are likely to use some of the tooling script locally in your contributing journey.
+See [plugin_tools](./script/tool/README.MD) for more details


Suggested change

See [plugin_tools](./script/tool/README.MD) for more details

See [plugin_tools](./script/tool/README.MD) for more details.

blasten · 2021-04-14T20:13:40Z

CONTRIBUTING.md

-pub global run flutter_plugin_tools format --plugins plugin_name
-pub global run flutter_plugin_tools analyze --plugins plugin_name
-pub global run flutter_plugin_tools test --plugins plugin_name
+$ cd script/tool & pub get


nit: no need for $

blasten · 2021-04-14T20:15:16Z

script/tool/README.md

+
+``sh
+cd <path_to_plugins>/packages
+dart run ./script/tool/lib/src/main.dart publish-plugin --package $package


what is $package? did you mean <package>?

blasten · 2021-04-14T20:16:45Z

script/tool/README.md

+By default the tool tries to push tags to the `upstream` remote, but that and
+some additional settings can be configured. Run `dart run ./script/tool/lib/src/main.dart publish-plugin --help` for more usage information.


"but that and some additional settings".

cyanglaz

@blasten Updated per your comments!

cyanglaz · 2021-04-15T19:46:26Z

CONTRIBUTING.md

-pub global run flutter_plugin_tools format --plugins plugin_name
-pub global run flutter_plugin_tools analyze --plugins plugin_name
-pub global run flutter_plugin_tools test --plugins plugin_name
+$ cd script/tool & pub get


cyanglaz · 2021-04-15T19:46:29Z

CONTRIBUTING.md

@@ -48,6 +48,14 @@ for any of the following plugins, we encourage you to submit it
   fetch from the master repository, not your clone, when running `git fetch`
   et al.)

+
+## Setting up tools
+


cyanglaz · 2021-04-15T19:46:33Z

CONTRIBUTING.md

+
+We use a range of tooling script to do varies of things on CI. (testing, formatting, etc.)
+Your are likely to use some of the tooling script locally in your contributing journey.
+See [plugin_tools](./script/tool/README.MD) for more details


cyanglaz · 2021-04-15T19:47:00Z

script/tool/README.md

+
+``sh
+cd <path_to_plugins>/packages
+dart run ./script/tool/lib/src/main.dart publish-plugin --package $package


cyanglaz · 2021-04-15T19:48:00Z

script/tool/README.md

+By default the tool tries to push tags to the `upstream` remote, but that and
+some additional settings can be configured. Run `dart run ./script/tool/lib/src/main.dart publish-plugin --help` for more usage information.


stuartmorgan · 2021-04-19T16:28:52Z

.github/PULL_REQUEST_TEMPLATE.md

@@ -8,7 +8,7 @@

 - [ ] 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 read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. (Note that unlike the flutter/flutter repo, we do use dart formatter in the flutter/plugins repo. See [plugin_tool format](../script/tool/README.md#format-code))


Nit: `dart format`

stuartmorgan · 2021-04-19T16:30:02Z

.github/PULL_REQUEST_TEMPLATE.md

@@ -8,7 +8,7 @@

 - [ ] 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 read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. (Note that unlike the flutter/flutter repo, we do use dart formatter in the flutter/plugins repo. See [plugin_tool format](../script/tool/README.md#format-code))


Nit: The structure would work better here if this said "Note that unlike the flutter/flutter repo, the flutter/plugins repo does use `dart format`."

stuartmorgan · 2021-04-19T16:31:44Z

CONTRIBUTING.md

+## Setting up tools
+
+We use a range of tooling script to do varies of things on CI. (testing, formatting, etc.)
+Your are likely to use some of the tooling script locally in your contributing journey.


How about just: "There are scripts for many common tasks (testing, formatting, etc.) that will likely be useful in preparing a PR.". It seems odd to frame it in terms of CI first when that's not the use case for most readers.

makes sense! Done

stuartmorgan · 2021-04-19T16:32:45Z

CONTRIBUTING.md

-pub global run flutter_plugin_tools test --plugins plugin_name
+ * Verify changes with [plugin_tools](./script/tool/README.MD).
+```sh
+cd script/tool && pub get


This is missing a cd back to the repo root.

stuartmorgan · 2021-04-19T16:33:09Z

CONTRIBUTING.md

+cd script/tool && pub get
+dart run ./script/tool/lib/src/main.dart format --plugins plugin_name
+pub run ./script/tool/lib/src/main.dart analyze --plugins plugin_name
+pub run ./script/tool/lib/src/main.dart test --plugins plugin_name


Shouldn't these all be dart?

stuartmorgan · 2021-04-19T16:35:28Z

script/tool/README.md

@@ -1,8 +1,60 @@
 # Flutter Plugin Tools

+Note: The commands in tools are designed to run under plugins/ or plugins/packages/.


s/under plugins/at the root of the repository/ (and similar for the second part); there's no guarantee that the local checkout is called packages even if it's the most likely scenario.

Done. The I assume you meant "there's no guarantee that the local checkout is called plugins". The packages folder is defined in the repo.

stuartmorgan · 2021-04-19T16:36:07Z

script/tool/README.md

@@ -1,8 +1,60 @@
 # Flutter Plugin Tools

+Note: The commands in tools are designed to run under plugins/ or plugins/packages/.
+
 To run the tool:

 ```sh
 dart pub get
 dart run lib/src/main.dart <args>


This command is misleading, given that as we've just said above this isn't the directory you should be in when running the tool.

stuartmorgan · 2021-04-19T16:36:51Z

script/tool/README.md

+## Format Code
+
+```sh
+cd <path_to_plugins>/packages


Remove "packages" from all of these examples; it's not necessary to be there, and it makes the next line wrong.

stuartmorgan · 2021-04-19T16:37:07Z

script/tool/README.md

+pub run ./script/tool/lib/src/main.dart analyze --plugins plugin_name
+```
+
+## Run dart unit tests


stuartmorgan · 2021-04-19T16:38:05Z

script/tool/README.md

+```
+
+## Publish and tag release
+


Mention here that you should be at the commit of the change being published? I was doing this wrong for quite a while.

done. added a git checkout command in the sh sample

stuartmorgan · 2021-04-19T17:17:04Z

script/tool/README.md

+
+``sh
+cd <path_to_plugins>
+git checkout <commit_hash_need_to_publish>


s/need_to/to/

stuartmorgan · 2021-04-19T17:17:27Z

script/tool/README.md

+additional settings can be configured. Run `dart run ./script/tool/lib/src/main.dart publish-plugin --help` for more usage information.
+
+The tool wraps `pub publish` for pushing the package to pub, and then will
+automatically use git to try and create and push tags. It has some additional


Nit: "to try to create"

stuartmorgan · 2021-04-19T17:18:28Z

CONTRIBUTING.md

+## Setting up tools
+
+We use a range of tooling script to do varies of things on CI. (testing, formatting, etc.)
+There are scripts for many common tasks (testing, formatting, etc.) that will likely be useful in preparing a PR.


You replied Done to the comment about rewriting this, but it doesn't seem to have been applied here.

oops, done.

stuartmorgan

LGTM!

ditman

This is a fantastic overhauling of the contributing process docs! Great stuff!

stuartmorgan · 2021-04-20T18:49:23Z

CONTRIBUTING.md

+## Setting up tools
+
+There are scripts for many common tasks (testing, formatting, etc.) that will likely be useful in preparing a PR.
+See [plugin_tools](./script/tool/README.MD) for more details.


Can you scrub this for all instances of .MD and replace them with .md? I just got a 404 from one of these links due to the incorrect capitalization.

Good catch! Thanks! Fix is here: #3815

* master: (397 commits) [in_app_purchase] Implementation of platform interface (flutter#3781) [google_sign_in] Add todo WRT correctly setting X-Goog-AuthUser header (flutter#3819) [tools] fix version check command not working for new packages (flutter#3818) [camera] android-rework part 1: Base classes to support Android Camera features (flutter#3795) fix MD (flutter#3815) Path provider windows crash fix (flutter#3814) [local_auth] docs update (flutter#3103) Update PULL_REQUEST_TEMPLATE.md (flutter#3801) [quick_actions] handle cold start on iOS correctly (flutter#3811) Replace path_provider_linux widget tests with simple unit tests (flutter#3812) [sensors] format dart code based on the new dart formatter (flutter#3809) [google_sign_in] Fix "pick account" on iOS (flutter#3805) [image_picker_platform_interface] Added pickMultiImage (flutter#3782) [in_app_purchase] Added currency code and numerical price to product detail model. (flutter#3794) [local_auth] Fix iOS crash when no localizedReason (flutter#3780) Fix and update version checks (flutter#3792) [in_app_purchase] Configured example app to use StoreKit Testing on iOS 14 (flutter#3772) [local_auth] Unnecessary reassignment in example removed (flutter#2983) [flutter_webview] Fix `allowsInlineMediaPlayback` ignored on iOS (flutter#3791) Switch script/tools over to the new analysis options (flutter#3777) ...

* master: (79 commits) Fix grammatical error in contributing guide (flutter#3217) [google_sign_in_web] fix README typos. [tool] combine run and runAndExitOnError (flutter#3827) [camera] android-rework part 2: Android auto focus feature (flutter#3796) [in_app_purchase_platform_interface] Added additional fields to ProductDetails (flutter#3826) Move all null safety packages' min dart sdk to 2.12.0 (flutter#3822) [path_provider_*] code cleanup: sort directives (flutter#3823) [in_app_purchase] Implementation of platform interface (flutter#3781) [google_sign_in] Add todo WRT correctly setting X-Goog-AuthUser header (flutter#3819) [tools] fix version check command not working for new packages (flutter#3818) [camera] android-rework part 1: Base classes to support Android Camera features (flutter#3795) fix MD (flutter#3815) Path provider windows crash fix (flutter#3814) [local_auth] docs update (flutter#3103) Update PULL_REQUEST_TEMPLATE.md (flutter#3801) [quick_actions] handle cold start on iOS correctly (flutter#3811) Replace path_provider_linux widget tests with simple unit tests (flutter#3812) [sensors] format dart code based on the new dart formatter (flutter#3809) [google_sign_in] Fix "pick account" on iOS (flutter#3805) [image_picker_platform_interface] Added pickMultiImage (flutter#3782) ...

Update PULL_REQUEST_TEMPLATE.md

1dd2374

Fixes flutter/flutter#80081

cyanglaz requested a review from stuartmorgan April 8, 2021 21:28

google-cla bot added the cla: yes label Apr 8, 2021

ditman reviewed Apr 8, 2021

View reviewed changes

Update PULL_REQUEST_TEMPLATE.md

802d1fe

blasten reviewed Apr 8, 2021

View reviewed changes

Chris Yang added 2 commits April 8, 2021 15:10

Update PULL_REQUEST_TEMPLATE.md

2c009a5

review

1569e8a

cyanglaz requested review from ditman and blasten April 14, 2021 18:11

fix typo

5ed2124

blasten reviewed Apr 14, 2021

View reviewed changes

review

c815fa2

cyanglaz commented Apr 15, 2021

View reviewed changes

Chris Yang added 3 commits April 15, 2021 12:53

fix

2c645ea

Merge branch 'master' into cyanglaz-patch-1

a771e18

Merge branch 'master' into cyanglaz-patch-1

a39a7a7

stuartmorgan suggested changes Apr 19, 2021

View reviewed changes

review

f7fe935

cyanglaz requested review from stuartmorgan and blasten April 19, 2021 17:01

stuartmorgan suggested changes Apr 19, 2021

View reviewed changes

review 2

8408aa3

cyanglaz requested a review from stuartmorgan April 19, 2021 17:23

stuartmorgan approved these changes Apr 19, 2021

View reviewed changes

Merge branch 'master' into cyanglaz-patch-1

74ef085

blasten approved these changes Apr 19, 2021

View reviewed changes

cyanglaz added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Apr 19, 2021

ditman approved these changes Apr 19, 2021

View reviewed changes

fluttergithubbot merged commit 6398441 into master Apr 20, 2021

engine-flutter-autoroll mentioned this pull request Apr 20, 2021

Roll Plugins from e2844d2373c6 to 6398441f5873 (1 revision) flutter/flutter#80786

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 20, 2021

6398441 Update PULL_REQUEST_TEMPLATE.md (flutter/plugins#3801)

629d0cf

ditman deleted the cyanglaz-patch-1 branch April 20, 2021 17:29

fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request Apr 20, 2021

6398441 Update PULL_REQUEST_TEMPLATE.md (flutter/plugins#3801) (#80786)

71cb40f

stuartmorgan reviewed Apr 20, 2021

View reviewed changes

cyanglaz mentioned this pull request Apr 20, 2021

fix incorrect MD in CONTRIBUTING.md #3815

Merged

fotiDim pushed a commit to fotiDim/plugins that referenced this pull request Sep 13, 2021

Update PULL_REQUEST_TEMPLATE.md (flutter#3801)

2ebad1c

	See [plugin_tools](./script/tool/README.MD) for more details
	See [plugin_tools](./script/tool/README.MD) for more details.

		By default the tool tries to push tags to the `upstream` remote, but that and
		some additional settings can be configured. Run `dart run ./script/tool/lib/src/main.dart publish-plugin --help` for more usage information.

		@@ -1,8 +1,60 @@
		# Flutter Plugin Tools

		Note: The commands in tools are designed to run under plugins/ or plugins/packages/.

Update PULL_REQUEST_TEMPLATE.md #3801

Update PULL_REQUEST_TEMPLATE.md #3801

Conversation

cyanglaz commented Apr 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz commented Apr 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

ditman left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ditman left a comment •

edited

Loading