Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

Update PULL_REQUEST_TEMPLATE.md #3801

Merged
merged 12 commits into from
Apr 20, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: `dart format`

Copy link
Contributor

Choose a reason for hiding this comment

The 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.
Expand Down
61 changes: 15 additions & 46 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra line

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


## Running the examples


Expand Down Expand Up @@ -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

Expand All @@ -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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no need for $

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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>`
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. Also removed the experimental verbiage

52 changes: 52 additions & 0 deletions script/tool/README.md
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/.
Copy link
Contributor

Choose a reason for hiding this comment

The 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 packages even if it's the most likely scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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>
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

```

## Format Code

```sh
cd <path_to_plugins>/packages
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. added a git checkout command in the sh sample

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"but that and some additional settings".

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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).