[flutter_plugin_tools] Validate pubspec description (flutter#4396)

stuartmorgan-g · KyleFin · commit 0fd2d6fa130f · 2021-12-21T10:14:55.000-07:00
pub.dev deducts points for having a pubspec.yaml `description` that is too short or too long; several of our plugins are losing points on this. To ensure that we are following—and modeling—best practices, this adds a check that our `description` fields meet pub.dev expectations.

Fixes our existing violations. Two are not published even though this only takes effect once published:
- camera: We change this plugin pretty frequently, so this should go out soon without adding a release just for this trivial issue.
- wifi_info_flutter: This is deprecated, so we don't plan to release it. It has to be fixed to allow the tool change to land though.
diff --git a/packages/camera/camera/CHANGELOG.md b/packages/camera/camera/CHANGELOG.md
@@ -1,3 +1,7 @@
+## NEXT
+
+* Updated package description.
+
 ## 0.9.4+1
 
 * Fixed Android implementation throwing IllegalStateException when switching to a different activity.
diff --git a/packages/camera/camera/pubspec.yaml b/packages/camera/camera/pubspec.yaml
@@ -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
diff --git a/packages/espresso/CHANGELOG.md b/packages/espresso/CHANGELOG.md
@@ -1,6 +1,7 @@
-## NEXT
+## 0.1.0+4
 
 * Updated Android lint settings.
+* Updated package description.
 
 ## 0.1.0+3
 
diff --git a/packages/espresso/pubspec.yaml b/packages/espresso/pubspec.yaml
@@ -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"
diff --git a/packages/file_selector/file_selector/CHANGELOG.md b/packages/file_selector/file_selector/CHANGELOG.md
@@ -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
 
diff --git a/packages/file_selector/file_selector/pubspec.yaml b/packages/file_selector/file_selector/pubspec.yaml
@@ -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"
diff --git a/packages/plugin_platform_interface/CHANGELOG.md b/packages/plugin_platform_interface/CHANGELOG.md
@@ -1,3 +1,7 @@
+## 2.0.2
+
+* Update package description.
+
 ## 2.0.1
 
 * Fix `federated flutter plugins` link in the README.md.
diff --git a/packages/plugin_platform_interface/pubspec.yaml b/packages/plugin_platform_interface/pubspec.yaml
@@ -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
 
@@ -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"
diff --git a/packages/wifi_info_flutter/wifi_info_flutter/CHANGELOG.md b/packages/wifi_info_flutter/wifi_info_flutter/CHANGELOG.md
@@ -1,6 +1,7 @@
 ## NEXT
 
 * Updated Android lint settings.
+* Updated package description.
 
 ## 2.0.2
 
diff --git a/packages/wifi_info_flutter/wifi_info_flutter/pubspec.yaml b/packages/wifi_info_flutter/wifi_info_flutter/pubspec.yaml
@@ -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
diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md
@@ -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
 
diff --git a/script/tool/lib/src/common/repository_package.dart b/script/tool/lib/src/common/repository_package.dart
@@ -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 =>
+      // 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');
diff --git a/script/tool/lib/src/pubspec_check_command.dart b/script/tool/lib/src/pubspec_check_command.dart
@@ -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;
@@ -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()
diff --git a/script/tool/test/common/repository_package_test.dart b/script/tool/test/common/repository_package_test.dart
@@ -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);
+    });
+  });
 }
diff --git a/script/tool/test/pubspec_check_command_test.dart b/script/tool/test/pubspec_check_command_test.dart
@@ -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/'
@@ -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' : ''}
@@ -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);
 
