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

[flutter_plugin_tools] Validate pubspec description #4396

Merged
merged 4 commits into from
Sep 30, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 4 additions & 0 deletions packages/camera/camera/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## NEXT

* Updated package description.

## 0.9.4+1

* Fixed Android implementation throwing IllegalStateException when switching to a different activity.
Expand Down
6 changes: 3 additions & 3 deletions packages/camera/camera/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name: camera
description: A Flutter plugin for getting information about and controlling the
camera on Android, iOS and Web. Supports previewing the camera feed, capturing images, capturing video,
and streaming image buffers to dart.
description: A Flutter plugin for controlling the camera. Supports previewing
the camera feed, capturing images and video, and streaming image buffers to
Dart.
repository: https://github.com/flutter/plugins/tree/master/packages/camera/camera
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+camera%22
version: 0.9.4+1
Expand Down
3 changes: 2 additions & 1 deletion packages/espresso/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
## NEXT
## 0.1.0+4

* Updated Android lint settings.
* Updated package description.

## 0.1.0+3

Expand Down
3 changes: 2 additions & 1 deletion packages/espresso/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
name: espresso
description: Java classes for testing Flutter apps using Espresso.
Allows driving Flutter widgets from a native Espresso test.
repository: https://github.com/flutter/plugins/tree/master/packages/espresso
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+espresso%22
version: 0.1.0+3
version: 0.1.0+4

environment:
sdk: ">=2.12.0 <3.0.0"
Expand Down
5 changes: 3 additions & 2 deletions packages/file_selector/file_selector/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
## NEXT
## 0.8.2+1

* Minor code cleanup for new analysis rules.
* Updated package description.

## 0.8.2

* Update platform_plugin_interface version requirement.
* Update `platform_plugin_interface` version requirement.

## 0.8.1

Expand Down
5 changes: 3 additions & 2 deletions packages/file_selector/file_selector/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
name: file_selector
description: Flutter plugin for opening and saving files.
description: Flutter plugin for opening and saving files, or selecting
directories, using native file selection UI.
repository: https://github.com/flutter/plugins/tree/master/packages/file_selector/file_selector
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+file_selector%22
version: 0.8.2
version: 0.8.2+1

environment:
sdk: ">=2.12.0 <3.0.0"
Expand Down
4 changes: 4 additions & 0 deletions packages/plugin_platform_interface/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 2.0.2

* Update package description.

## 2.0.1

* Fix `federated flutter plugins` link in the README.md.
Expand Down
5 changes: 3 additions & 2 deletions packages/plugin_platform_interface/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
name: plugin_platform_interface
description: Reusable base class for Flutter plugin platform interfaces.
description: Reusable base class for platform interfaces of Flutter federated
plugins, to help enforce best practices.
repository: https://github.com/flutter/plugins/tree/master/packages/plugin_platform_interface
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+plugin_platform_interface%22

Expand All @@ -14,7 +15,7 @@ issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+
# be done when absolutely necessary and after the ecosystem has already migrated to 1.X.Y version
# that is forward compatible with 2.0.0 (ideally the ecosystem have migrated to depend on:
# `plugin_platform_interface: >=1.X.Y <3.0.0`).
version: 2.0.1
version: 2.0.2

environment:
sdk: ">=2.12.0 <3.0.0"
Expand Down
1 change: 1 addition & 0 deletions packages/wifi_info_flutter/wifi_info_flutter/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
## NEXT

* Updated Android lint settings.
* Updated package description.

## 2.0.2

Expand Down
2 changes: 1 addition & 1 deletion packages/wifi_info_flutter/wifi_info_flutter/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: wifi_info_flutter
description: A new flutter plugin project.
description: A Flutter plugin to get WiFi information such as connection status and network identifiers.
repository: https://github.com/flutter/plugins/tree/master/packages/wifi_info_flutter/wifi_info_flutter
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+wifi_info_flutter%22
version: 2.0.2
Expand Down
2 changes: 2 additions & 0 deletions script/tool/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
major version change restriction.
- Improved error handling and error messages in CHANGELOG version checks.
- `license-check` now validates Kotlin files.
- `pubspec-check` now checks that the description is of the pub-recommended
length.

## 0.7.1

Expand Down
9 changes: 9 additions & 0 deletions script/tool/lib/src/common/repository_package.dart
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,15 @@ class RepositoryPackage {
bool get isPlatformInterface =>
directory.basename.endsWith('_platform_interface');

/// True if this appears to be a platform implementation package, according to
/// repository conventions.
bool get isPlatformImplementation =>
Copy link
Member

Choose a reason for hiding this comment

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

Should this be determined by the presence of the implements pubspec.yaml property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we ever find that this heuristic isn't working, that's the way to go (and by having it centralized here was can easily improve it later). That's way more involved though since it requires loading and parsing the pubspec, and I suspect the heuristic may well be all we ever need.

// Any part of a federated plugin that isn't the platform interface and
// isn't the app-facing package should be an implementation package.
isFederated &&
!isPlatformInterface &&
directory.basename != directory.parent.basename;

/// Returns the Flutter example packages contained in the package, if any.
Iterable<RepositoryPackage> getExamples() {
final Directory exampleDirectory = directory.childDirectory('example');
Expand Down
33 changes: 33 additions & 0 deletions script/tool/lib/src/pubspec_check_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,18 @@ class PubspecCheckCommand extends PackageLoopingCommand {
'${indentation * 2}$_expectedIssueLinkFormat<package label>');
passing = false;
}

// Don't check descriptions for federated package components other than
// the app-facing package, since they are unlisted, and are expected to
// have short descriptions.
if (!package.isPlatformInterface && !package.isPlatformImplementation) {
final String? descriptionError =
_checkDescription(pubspec, package: package);
if (descriptionError != null) {
printError('$indentation$descriptionError');
passing = false;
}
}
}

return passing;
Expand Down Expand Up @@ -180,6 +192,27 @@ class PubspecCheckCommand extends PackageLoopingCommand {
return errorMessages;
}

// Validates the "description" field for a package, returning an error
// string if there are any issues.
String? _checkDescription(
Pubspec pubspec, {
required RepositoryPackage package,
}) {
final String? description = pubspec.description;
if (description == null) {
return 'Missing "description"';
}

if (description.length < 60) {
return '"description" is too short. pub.dev recommends package '
'descriptions of 60-180 characters.';
}
if (description.length > 180) {
return '"description" is too long. pub.dev recommends package '
'descriptions of 60-180 characters.';
}
}

bool _checkIssueLink(Pubspec pubspec) {
return pubspec.issueTracker
?.toString()
Expand Down
35 changes: 35 additions & 0 deletions script/tool/test/common/repository_package_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -120,4 +120,39 @@ void main() {
plugin.childDirectory('example').childDirectory('example2').path);
});
});

group('federated plugin queries', () {
test('all return false for a simple plugin', () {
final Directory plugin = createFakePlugin('a_plugin', packagesDir);
expect(RepositoryPackage(plugin).isFederated, false);
expect(RepositoryPackage(plugin).isPlatformInterface, false);
expect(RepositoryPackage(plugin).isFederated, false);
});

test('handle app-facing packages', () {
final Directory plugin =
createFakePlugin('a_plugin', packagesDir.childDirectory('a_plugin'));
expect(RepositoryPackage(plugin).isFederated, true);
expect(RepositoryPackage(plugin).isPlatformInterface, false);
expect(RepositoryPackage(plugin).isPlatformImplementation, false);
});

test('handle platform interface packages', () {
final Directory plugin = createFakePlugin('a_plugin_platform_interface',
packagesDir.childDirectory('a_plugin'));
expect(RepositoryPackage(plugin).isFederated, true);
expect(RepositoryPackage(plugin).isPlatformInterface, true);
expect(RepositoryPackage(plugin).isPlatformImplementation, false);
});

test('handle platform implementation packages', () {
// A platform interface can end with anything, not just one of the known
// platform names, because of cases like webview_flutter_wkwebview.
final Directory plugin = createFakePlugin(
'a_plugin_foo', packagesDir.childDirectory('a_plugin'));
expect(RepositoryPackage(plugin).isFederated, true);
expect(RepositoryPackage(plugin).isPlatformInterface, false);
expect(RepositoryPackage(plugin).isPlatformImplementation, true);
});
});
}
93 changes: 93 additions & 0 deletions script/tool/test/pubspec_check_command_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ void main() {
bool includeHomepage = false,
bool includeIssueTracker = true,
bool publishable = true,
String? description,
}) {
final String repositoryPath = repositoryPackagesDirRelativePath ?? name;
final String repoLink = 'https://github.com/flutter/'
Expand All @@ -64,8 +65,11 @@ void main() {
final String issueTrackerLink =
'https://github.com/flutter/flutter/issues?'
'q=is%3Aissue+is%3Aopen+label%3A%22p%3A+$name%22';
description ??= 'A test package for validating that the pubspec.yaml '
'follows repo best practices.';
return '''
name: $name
description: $description
${includeRepository ? 'repository: $repoLink' : ''}
${includeHomepage ? 'homepage: $repoLink' : ''}
${includeIssueTracker ? 'issue_tracker: $issueTrackerLink' : ''}
Expand Down Expand Up @@ -327,6 +331,95 @@ ${devDependenciesSection()}
);
});

test('fails when description is too short', () async {
final Directory pluginDirectory =
createFakePlugin('a_plugin', packagesDir.childDirectory('a_plugin'));

pluginDirectory.childFile('pubspec.yaml').writeAsStringSync('''
${headerSection('plugin', isPlugin: true, description: 'Too short')}
${environmentSection()}
${flutterSection(isPlugin: true)}
${dependenciesSection()}
${devDependenciesSection()}
''');

Error? commandError;
final List<String> output = await runCapturingPrint(
runner, <String>['pubspec-check'], errorHandler: (Error e) {
commandError = e;
});

expect(commandError, isA<ToolExit>());
expect(
output,
containsAllInOrder(<Matcher>[
contains('"description" is too short. pub.dev recommends package '
'descriptions of 60-180 characters.'),
]),
);
});

test(
'allows short descriptions for non-app-facing parts of federated plugins',
() async {
final Directory pluginDirectory = createFakePlugin('plugin', packagesDir);

pluginDirectory.childFile('pubspec.yaml').writeAsStringSync('''
${headerSection('plugin', isPlugin: true, description: 'Too short')}
${environmentSection()}
${flutterSection(isPlugin: true)}
${dependenciesSection()}
${devDependenciesSection()}
''');

Error? commandError;
final List<String> output = await runCapturingPrint(
runner, <String>['pubspec-check'], errorHandler: (Error e) {
commandError = e;
});

expect(commandError, isA<ToolExit>());
expect(
output,
containsAllInOrder(<Matcher>[
contains('"description" is too short. pub.dev recommends package '
'descriptions of 60-180 characters.'),
]),
);
});

test('fails when description is too long', () async {
final Directory pluginDirectory = createFakePlugin('plugin', packagesDir);

const String description = 'This description is too long. It just goes '
'on and on and on and on and on. pub.dev will down-score it because '
'there is just too much here. Someone shoul really cut this down to just '
'the core description so that search results are more useful and the '
'package does not lose pub points.';
pluginDirectory.childFile('pubspec.yaml').writeAsStringSync('''
${headerSection('plugin', isPlugin: true, description: description)}
${environmentSection()}
${flutterSection(isPlugin: true)}
${dependenciesSection()}
${devDependenciesSection()}
''');

Error? commandError;
final List<String> output = await runCapturingPrint(
runner, <String>['pubspec-check'], errorHandler: (Error e) {
commandError = e;
});

expect(commandError, isA<ToolExit>());
expect(
output,
containsAllInOrder(<Matcher>[
contains('"description" is too long. pub.dev recommends package '
'descriptions of 60-180 characters.'),
]),
);
});

test('fails when environment section is out of order', () async {
final Directory pluginDirectory = createFakePlugin('plugin', packagesDir);

Expand Down