feature: tag tests; choose tags on command line

lib/src/backend/metadata.dart

Index: lib/src/backend/metadata.dart
diff --git a/lib/src/backend/metadata.dart b/lib/src/backend/metadata.dart
index d3991e23e884d7b66478f46bf9d974797ffe70c2..b8e6c0c6ace884bc0e707d870e6c8f775291b599 100644
--- a/lib/src/backend/metadata.dart
+++ b/lib/src/backend/metadata.dart
@@ -34,7 +34,7 @@ class Metadata {
   final String skipReason;
 
   /// A set of tags attached to a test
-  final List<String> tags;
+  final Set<String> tags;
 
   /// Platform-specific metadata.
   ///
@@ -91,14 +91,14 @@ class Metadata {
   /// [testOn] defaults to [PlatformSelector.all].
   Metadata({PlatformSelector testOn, Timeout timeout, bool skip: false,
           this.verboseTrace: false, this.skipReason,
-          Map<PlatformSelector, Metadata> onPlatform, List<String> tags})
+          Map<PlatformSelector, Metadata> onPlatform, tags})
       : testOn = testOn == null ? PlatformSelector.all : testOn,
         timeout = timeout == null ? const Timeout.factor(1) : timeout,
         skip = skip,
         onPlatform = onPlatform == null
             ? const {}
             : new UnmodifiableMapView(onPlatform),
-        this.tags = tags ?? const <String>[];
+        this.tags = _makeSetOfTags(tags);

[nweiz, 2015/11/03 00:43:16]

The type-normalization should only happen in [new Metadata.parse]. [new
Metadata] should just take an Iterable<String>, and [new Metadata.deserialize]
should assume it will deserialize to a list.

[yjbanov, 2015/11/11 06:40:20]

Done.

 
   /// Creates a new Metadata, but with fields parsed from caller-friendly values
   /// where applicable.
@@ -106,7 +106,7 @@ class Metadata {
   /// Throws a [FormatException] if any field is invalid.
   Metadata.parse({String testOn, Timeout timeout, skip,
           this.verboseTrace: false, Map<String, dynamic> onPlatform,
-          List<String> tags})
+          tags})
       : testOn = testOn == null
             ? PlatformSelector.all
             : new PlatformSelector.parse(testOn),
@@ -114,14 +114,14 @@ class Metadata {
         skip = skip != null && skip != false,
         skipReason = skip is String ? skip : null,
         onPlatform = _parseOnPlatform(onPlatform),
-        this.tags = tags ?? const <String>[] {
+        this.tags = _makeSetOfTags(tags) {
     if (skip != null && skip is! String && skip is! bool) {
       throw new ArgumentError(
           '"skip" must be a String or a bool, was "$skip".');
     }
   }
 
-  /// Dezerializes the result of [Metadata.serialize] into a new [Metadata].
+  /// Deserializes the result of [Metadata.serialize] into a new [Metadata].
   Metadata.deserialize(serialized)
       : testOn = serialized['testOn'] == null
             ? PlatformSelector.all
@@ -130,7 +130,7 @@ class Metadata {
         skip = serialized['skip'],
         skipReason = serialized['skipReason'],
         verboseTrace = serialized['verboseTrace'],
-        tags = serialized['tags'] ?? const <String>[],
+        tags = _makeSetOfTags(serialized['tags']),
         onPlatform = new Map.fromIterable(serialized['onPlatform'],
             key: (pair) => new PlatformSelector.parse(pair.first),
             value: (pair) => new Metadata.deserialize(pair.last));
@@ -155,7 +155,7 @@ class Metadata {
           verboseTrace: verboseTrace || other.verboseTrace,
           skipReason: other.skipReason == null ? skipReason : other.skipReason,
           onPlatform: mergeMaps(onPlatform, other.onPlatform),
-          tags: mergeLists(tags, other.tags));
+          tags: tags.union(other.tags));
 
   /// Returns a copy of [this] with the given fields changed.
   Metadata change({PlatformSelector testOn, Timeout timeout, bool skip,
@@ -201,7 +201,7 @@ class Metadata {
       'skipReason': skipReason,
       'verboseTrace': verboseTrace,
       'onPlatform': serializedOnPlatform,
-      'tags': tags.isEmpty ? null : tags,
+      'tags': tags.toList(),
     };
   }
 
@@ -216,3 +216,34 @@ class Metadata {
     };
   }
 }
+
+/// Converts [tags] to a [Set] of unique tags, unless it is already a [Set].
+///
+/// `null` becomes an empty set. [String] is split on commas.
+Set<String> _makeSetOfTags(tags) => tags != null
+  ? tags is Iterable
+    ? _tagsFromIterable(tags)
+    : tags is String
+      ? _tagsFromString(tags)
+      : throw new ArgumentError.value(tags, "tags",
+          "must be either String or Iterable")
+  : new Set<String>();
+
+Set<String> _tagsFromIterable(Iterable tags) {
+  for (var tag in tags) {
+    if (tag is! String) {
+      throw new ArgumentError.value(tags, "tags", "tag name must be String");
+    }
+    if (tag.trim().isEmpty) {

[nweiz, 2015/11/03 00:43:16]

I'd rather not trim here. If users want to use whitespace tags or even empty
tags, I don't see why they shouldn't.

[yjbanov, 2015/11/11 06:40:20]

I thought it would be more practical given we accept tags as command-line
arguments, where one has to take a moment to figure out how to properly pass
leading/trailing whitespace, and because we do not lose any generality either
way. However, I'll remove it if you still think leading/trailing whitespace is
useful somehow.

[nweiz, 2015/11/16 21:59:44]

I'd like to keep the semantics on the Dart side independent of the specifics of
parsing command-line arguments.

[yjbanov, 2015/11/26 06:30:27]

Done.

+      throw new ArgumentError.value(tags, "tags",
+          "tag name must contain non-whitespace characters");
+    }
+  }
+  return tags is Set<String> ? tags : new Set<String>.from(tags);
+}
+
+Set<String> _tagsFromString(String tags) {
+  if (tags.trim().isEmpty) return new Set<String>();
+  return _tagsFromIterable(
+      tags.split(",").map((tag) => tag.trim()).where((tag) => tag.isNotEmpty));

[nweiz, 2015/11/03 00:43:16]

I'd rather require users to use a real list if they want multiple tags. The less
parsing we do, the less users need to remember how the specific parsing works.

[yjbanov, 2015/11/11 06:40:20]

I thought it would make it consistent with the command-line arguments where we
do accept comma-separated tags, and again, one would have to think longer to
figure out how to pass a tag with a comma in the name without having ArgParser
split it into multiple tags.

[yjbanov, 2015/11/26 06:30:27]

Done.

+}