-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[flutter_plugin_tools] Improve package targeting #4577
[flutter_plugin_tools] Improve package targeting #4577
Conversation
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.
I have a few nits on documentation, code and tests look good.
- `--packages` now allows using a federated plugin's package as a target without | ||
fully specifying it (if it is not the same as the plugin's name). E.g., |
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.
Should this information be in the help page for the tool?
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.
Good idea, done.
// if .../packages/parentName/candidatePackageName/... | ||
// looks like a path in a federated plugin package (candidatePackageName) | ||
// rather than a top-level package (parentName). | ||
bool isFederatedPackage(String candidatePackageName, String parentName) { |
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.
s/isFederatedPackage/_isFederatedPackage
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.
Locals shouldn't be named with underscores.
// A helper function that returns true if candidateName looks like an | ||
// implementation package of a plugin called pluginName. Used to determine | ||
// if .../packages/parentName/candidatePackageName/... | ||
// looks like a path in a federated plugin package (candidatePackageName) | ||
// rather than a top-level package (parentName). |
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.
You might as well make this a docstring so you get compiler support about variable names and what not. There is already a mixmatch between "candidateName" and "candidatePackageName"
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.
This is a locally scoped function, which doesn't seem to support docstrings (at least not in VS Code). I fixed candidateName
though.
This pull request is not suitable for automatic merging in its current state.
|
Overriding incorrect submit-queue check; it's pulling status from the previous publish run, not the current (successful) run. |
Improve package targeting: - `--run-on-changed-packages` now includes only changed packages in a federated plugin, not all packages. Include all packages isn't useful since (without changes that would cause them to be included anyway) package dependencies are not path-based between packages. - `--packages` now allows specifying package names of federated plugins. E.g., `--packages=path_provider_ios` will now work, instead of requiring `--packages=path_provider/path_provider_ios`. The fully qualified form still works as well (and is still needed for app-facing packages to disambiguate from the plugin group). Fixes flutter/flutter#94618
Improve package targeting: - `--run-on-changed-packages` now includes only changed packages in a federated plugin, not all packages. Include all packages isn't useful since (without changes that would cause them to be included anyway) package dependencies are not path-based between packages. - `--packages` now allows specifying package names of federated plugins. E.g., `--packages=path_provider_ios` will now work, instead of requiring `--packages=path_provider/path_provider_ios`. The fully qualified form still works as well (and is still needed for app-facing packages to disambiguate from the plugin group). Fixes flutter/flutter#94618
Improve package targeting:
--run-on-changed-packages
now includes only changed packages in a federated plugin, not all packages. Include all packages isn't useful since (without changes that would cause them to be included anyway) package dependencies are not path-based between packages.--packages
now allows specifying package names of federated plugins. E.g.,--packages=path_provider_ios
will now work, instead of requiring--packages=path_provider/path_provider_ios
. The fully qualified form still works as well (and is still needed for app-facing packages to disambiguate from the plugin group).Fixes flutter/flutter#94618
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).