-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Update PULL_REQUEST_TEMPLATE.md #3801
Changes from 5 commits
1dd2374
802d1fe
2c009a5
1569e8a
5ed2124
c815fa2
2c645ea
a771e18
a39a7a7
f7fe935
8408aa3
74ef085
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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`." |
||
- [ ] I signed the [CLA]. | ||
- [ ] The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. `[shared_preferences]` | ||
- [ ] I listed at least one issue that this PR fixes in the description above. | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 | ||||||
|
||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: extra line There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||||||
|
||||||
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. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. makes sense! Done |
||||||
See [plugin_tools](./script/tool/README.MD) for more details | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||||||
|
||||||
## Running the examples | ||||||
|
||||||
|
||||||
|
@@ -137,13 +145,7 @@ pod lib lint --allow-warnings | |||||
XCUITests aren't usually configured with cocoapods in this repo. They are configured in a xcode workspace target named RunnerUITests. | ||||||
To run all the XCUITests in a plugin, follow the steps in a regular iOS development workflow [here](https://developer.apple.com/library/archive/documentation/DeveloperTools/Conceptual/testing_with_xcode/chapters/05-running_tests.html) | ||||||
|
||||||
For convenience, a [flutter_plugin_tools](https://pub.dev/packages/flutter_plugin_tools) command `xctest` could also be used to run all the XCUITests in the repo: | ||||||
|
||||||
```console | ||||||
pub global activate flutter_plugin_tools | ||||||
cd <path_to_plugins>/packages | ||||||
pub global run flutter_plugin_tools xctest --target RunnerUITests --skip <plugins_to_skip> | ||||||
``` | ||||||
For convenience, a [plugin_tools](./script/tool/README.MD) command [xctest](./script/tool/README.MD#run-xctests) could also be used to run all the XCUITests in the repo. | ||||||
|
||||||
## Contributing code | ||||||
|
||||||
|
@@ -159,12 +161,12 @@ To start working on a patch: | |||||
* `git fetch upstream` | ||||||
* `git checkout upstream/master -b <name_of_your_branch>` | ||||||
* Hack away. | ||||||
* Verify changes with [flutter_plugin_tools](https://pub.dev/packages/flutter_plugin_tools) | ||||||
* Verify changes with [plugin_tools](./script/tool/README.MD). | ||||||
``` | ||||||
pub global activate flutter_plugin_tools | ||||||
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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: no need for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||||||
$ 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 | ||||||
``` | ||||||
* `git commit -a -m "<your informative commit message>"` | ||||||
* `git push origin <name_of_your_branch>` | ||||||
|
@@ -245,37 +247,4 @@ Releasing a package is a two-step process. | |||||
done manually with `git tag $tagname && git push upstream $tagname` while | ||||||
checked out on the commit that updated `version` in `pubspec.yaml`. | ||||||
|
||||||
We've recently updated | ||||||
[flutter_plugin_tools](https://github.com/flutter/plugin_tools) to wrap both of | ||||||
those steps into one command to make it a little easier. This new tool is | ||||||
experimental. Feel free to fall back on manually running `pub publish` and | ||||||
creating and pushing the tag in git if there are issues with it. | ||||||
|
||||||
Install the tool by running: | ||||||
|
||||||
```terminal | ||||||
$ pub global activate flutter_plugin_tools | ||||||
``` | ||||||
|
||||||
Then, from the root of your local `flutter/plugins` repo, use the tool to | ||||||
publish a release. | ||||||
|
||||||
```terminal | ||||||
$ pub global run flutter_plugin_tools publish-plugin --package $package | ||||||
``` | ||||||
|
||||||
By default the tool tries to push tags to the `upstream` remote, but that and | ||||||
some additional settings can be configured. Run `pub global activate | ||||||
flutter_plugin_tools --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 | ||||||
safety checking around `pub publish` too. By default `pub publish` publishes | ||||||
_everything_, including untracked or uncommitted files in version control. | ||||||
`flutter_plugin_tools publish-plugin` will first check the status of the local | ||||||
directory and refuse to publish if there are any mismatched files with version | ||||||
control present. | ||||||
|
||||||
There is a lot about this process that is still to be desired. Some top level | ||||||
items are being tracked in | ||||||
[flutter/flutter#27258](https://github.com/flutter/flutter/issues/27258). | ||||||
For convenience, a [publish-plugin](./script/tool/README.MD#publish-and-tag-release) tool script could be used to do both step at once. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's flip this section to say to use this script, and then give the manual steps as fallback. We should lead with the option we want people to use, and this is no longer experimental given that we use it all the time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. Also removed the experimental verbiage |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,60 @@ | ||
# Flutter Plugin Tools | ||
|
||
Note: The commands in tools are designed to run under plugins/ or plugins/packages/. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
To run the tool: | ||
|
||
```sh | ||
dart pub get | ||
dart run lib/src/main.dart <args> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
``` | ||
|
||
## Format Code | ||
|
||
```sh | ||
cd <path_to_plugins>/packages | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove "packages" from all of these examples; it's not necessary to be there, and it makes the next line wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
dart run ./script/tool/lib/src/main.dart format --plugins plugin_name | ||
``` | ||
|
||
## Run static analyzer | ||
|
||
```sh | ||
cd <path_to_plugins>/packages | ||
pub run ./script/tool/lib/src/main.dart analyze --plugins plugin_name | ||
``` | ||
|
||
## Run dart unit tests | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Dart |
||
|
||
```sh | ||
cd <path_to_plugins>/packages | ||
pub run ./script/tool/lib/src/main.dart test --plugins plugin_name | ||
``` | ||
|
||
## Run XCTests | ||
|
||
```sh | ||
cd <path_to_plugins>/packages | ||
dart run lib/src/main.dart xctest --target RunnerUITests --skip <plugins_to_skip> | ||
``` | ||
|
||
## Publish and tag release | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mention here that you should be at the commit of the change being published? I was doing this wrong for quite a while. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. added a |
||
``sh | ||
cd <path_to_plugins>/packages | ||
dart run ./script/tool/lib/src/main.dart publish-plugin --package $package | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, done |
||
`` | ||
|
||
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "but that and some additional settings". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: "to try to create" |
||
safety checking around `pub publish` too. By default `pub publish` publishes | ||
_everything_, including untracked or uncommitted files in version control. | ||
`publish-plugin` will first check the status of the local | ||
directory and refuse to publish if there are any mismatched files with version | ||
control present. | ||
|
||
There is a lot about this process that is still to be desired. Some top level | ||
items are being tracked in | ||
[flutter/flutter#27258](https://github.com/flutter/flutter/issues/27258). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: `dart format`