Add GroupSet and SetGroup classes.

lib/src/set_group.dart

+// Copyright (c) 2016, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'group_set.dart';
+import 'wrappers.dart';
+
+/// A set of sets that provides [set], a view of the union of those sets.
+///
+/// This is a convenience class for creating a [GroupSet] whose contents change
+/// over the lifetime of a class. For example:
+///
+/// ```dart
+/// class Engine {
+///   Set<Test> get activeTests => _activeTestsGroup.set;
+///   final _activeTestsGroup = new SetGroup<Test>();
+///
+///   void addSuite(Suite suite) {
+///     _activeTestsGroup.add(suite.tests);
+///     _runSuite(suite);
+///     _activeTestsGroup.remove(suite.tests);
+///   }
+/// }
+/// ```
+class SetGroup<E> extends DelegatingSet<Set<E>> {

[Lasse Reichstein Nielsen, 2016/04/23 09:00:47]

I do think I'd prefer this class to not be a Set.

I can still have add, remove and contains methods, but probably nothing else.
It may have a Set getter (Set<Set<E>> get sets) that gives a Set view, but the
class itself should not be a Set - it's role is to control the GroupSet, not to
*be* a Set. That also avoids extending the Set interface.

[floitsch, 2016/04/25 16:11:10]

Do we even need this class?

I see two ways to avoid it:

Add 'addSet', 'removeSet' to the `GroupSet`.

Keep the set of sets that are used to initialize the `GroupSet`.

class Engine {
  Set<Test> get activeTests => _activeTestsGroup.set;
  final Set<Set<Test>> _activeTestSets = new Set<Set<Test>>();
  get _activeTestsGroup = new SetGroup<Test>(_activeTestSets);

  void addSuite(Suite suite) {
    _activeTestSets.add(suite.tests);
    _runSuite(suite);
    _activeTestSets.remove(suite.tests);
  }
}

In any case, the naming is very confusing.

[nweiz, 2016/04/26 21:23:53]

Done.
It's important that GroupSet *not* have additional methods that aren't in Set.
The fact that a Set exposed by a class is implemented using GroupSet should be
invisible to the user.
Creating a new SetGroup for every call to [activeTests] is a lot of overhead and
indirection that would be avoided by this class existing.

[floitsch, 2016/04/27 10:56:52]

Why?
It's like saying that users expecting an "Iterable" must never get anything that
has more methods.
With strong mode you can't even access these additional methods accidentally.
Then cache it.
Fwiw I strongly prefer the 'additional methods' approach.

[Lasse Reichstein Nielsen, 2016/04/27 12:17:53]

I generally recommend not extending the well-known interfaces like List, Set,
Future, etc.
Iterable is special because it is intended as a superclass of collections in
general. I wouldn't recommend anybody extending Iterable without good cause -
like creating a new kind of collection that makes sense by itself - and not just
adding a single method that makes sense in a single setting.

An interface of "Set + two more methods" is an unnecessary mental overhead for
the user. Either it's a Set or it's SomethingElse, and that SomethingElse should
only have the members necessary to fill its role, and not worry about being a
Set as well.
I very much prefer something that *has* those two methods and *has* a set, to a
Set that has those methods as well.

[floitsch, 2016/04/27 12:31:20]

I'm not convinced.
A UnionSet supports adding complete sets without copying them (using the
argument as backing set). As such it *is* a Set and has operations that are just
not supported by the base Set. Extending it and adding the additional methods
sounds like a perfect use case of subclassing.

The GroupSet now just gives access to (currently) private methods of the
UnionSet. I don't think that separation is necessary, and that the methods
should just be exposed (especially since the new methods are very similar to
existing ones like `add` or `addAll`).

[Lasse Reichstein Nielsen, 2016/04/27 12:47:00]

The GroupSet is just a Set at the interface level. There doesn't need to be a
visible *GroupSet* class at all - that's just an implementation detail and a
place to stick the constructors.
You might be able to add a set to it, but that too is an implementation detail
that is only available to the creator of the GroupSet.

If you add methods to that Set then it's a new kind of Set, not just a plain
Set. Any place it's passed to can check the type and cast it to get access to
the new methods. You'll likely see some use-cases where the GroupSet is wrapped
in a SetView to *hide* the extra methods.

I prefer a GroupSetController with a set getter and the methods that allow you
access to the implementation as necessary. That's what we did in other places,
and for the same reasons.

[floitsch, 2016/04/27 13:08:47]

If that's really necessary, yes. That's how it works. It's the same reason we
have wrappers that disallow writing operations to Set.
We do have some cases (Futures and Streams) where we did that separation. But
the separated classes have fundamentally different purposes. One is usually the
output. The other the input.
Here, we just add new methods with similar functionality as the existing ones.
`addSet` really is the same as `addAll`, just that it uses the given set as a
backing set (without copying the elements).
If you are really afraid that users want to modify the set, then you have to
wrap it (as with any other Set).
I understand that the UnionSet is, by default, an unmodifiable set, but that
seems to be more of an implementation issue than a fundamental property of
UnionSet.

I see the UnionSet much more as an extension to Set than a different class.
To me UnionSet is to Set, like EventSink to Sink. Both simply add new methods
that add new functionality.

[Lasse Reichstein Nielsen, 2016/04/27 14:31:15]

I think our difference is mainly in perspective. I see the GroupSet as a *Set
view* of a collection of Sets. It's not interesting in itself, anybody seeing it
will just see a Set. 
You see it as a new kind of collection that allows adding entire sets in
constant time. 
I can't find a good reason for having that kind of collection as a collection
(rather than a view), but I'll defer to Natalie on what the actual original
intent was - a new kind of collection or a view that looks like a Set.

[nweiz, 2016/04/27 20:47:40]

The intent was definitely a view that looks like a set. I'm coming at this from
the perspective of exposing information about an existing class, and the
controller/external object duality works very well for that use case.

+  /// The [GroupSet] that provides a view of the union of sets in [this].
+  GroupSet<E> get set => _set;
+  GroupSet<E> _set;

[floitsch, 2016/04/25 16:11:10]

This should be final.

[nweiz, 2016/04/26 21:23:53]

Unfortunately it can't be; GroupSet's initializer refers to [this].

[floitsch, 2016/04/27 10:56:52]

True. It was, but isn't anymore.

Looking more at it, I think, it is not a good approach to make it possible to
change the set after it has been passed to the UnionSet (thus my strong
preference to having the additional methods).

If we keep the UnionSetController, then in should be in the same library as the
UnionSet and just change the UnionSet's private variable.

[nweiz, 2016/04/27 20:47:40]

Allowing a UnionSet to be directly based on an existing Set<Set> seems strictly
more expressive than forcing it to only be created using a controller. It also
matches the APIs of existing view classes like MapKeySet and MapValueSet. I'm
not sure why you wouldn't want to allow it.

[Lasse Reichstein Nielsen, 2016/04/27 21:03:30]

Allowing a UnionSet to be directly based on an existing Set<Set> is fine (with
me). It's the SetGroup/UnionSetController class I'm concerned about - the one
that creates a UnionSet and a way to modify it in one go. 

That one could be a controller style class (getter for UnionSet, operations that
control the underlying set), or it can be just a class that just exposes the
UnionSet and the Set<Set>. (I just don't want one to be a property of the other,
nor the UnionSet to expose the Set<Set> below it and break the Set interface).

[nweiz, 2016/05/03 01:05:36]

Right now it's the former: a class that exposes a getter for a UnionSet and
methods to control that set.

[Lasse Reichstein Nielsen, 2016/05/03 08:27:13]

Yes, so I'm good too :)

+
+  /// Creates a set of sets that provides a view of the union of those sets.
+  ///
+  /// If [disjoint] is `true`, this assumes that all component sets are
+  /// disjoint—that is, that they contain no elements in common. This makes
+  /// many operations including [length] more efficient.
+  SetGroup({bool disjoint: false}) : super(new Set<Set<E>>()) {
+    _set = new GroupSet<E>(this, disjoint: disjoint);
+  }
+}