editing doc comments for List class

sdk/lib/core/list.dart

Index: sdk/lib/core/list.dart
diff --git a/sdk/lib/core/list.dart b/sdk/lib/core/list.dart
index dc0ccfab1bed944dec904c041669fce5bdc27725..af9f8950f238f7b6988a546ff7693c02b9da66ac 100644
--- a/sdk/lib/core/list.dart
+++ b/sdk/lib/core/list.dart
@@ -5,72 +5,66 @@
 part of dart.core;
 
 /**
- * A [List] is an indexable collection with a length.
+ * An indexable collection of objects with a length.
  *
- * A `List` implementation can choose not to support all methods
- * of the `List` interface.
+ * Subclasses of this class implement different kinds of lists.
+ * The most common kinds of lists are:
  *
- * The most common list types are:
- * * Fixed length list. It is an error to use operations that can change
- *   the list's length.
- * * Growable list. Full implementation of the interface.
- * * Unmodifiable list. It is an error to use operations that can change
- *   the list's length, or that can change the values of the list.
- *   If an unmodifable list is backed by another modifiable data structure,
- *   the values read from it may still change over time.
+ * * Fixed-length list.
+ *   An error occurs when attempting to use operations
+ *   that can change the length of the list.
  *
- * Example:
+ * * Growable list. Full implementation of the API defined in this class.
+ *
+ * The following code illustrates that some List implementations support
+ * only a subset of the API.
  *
  *     var fixedLengthList = new List(5);
- *     fixedLengthList.length = 0;  // throws.
- *     fixedLengthList.add(499);  // throws
+ *     fixedLengthList.length = 0;  // Error.
+ *     fixedLengthList.add(499);    // Error.
  *     fixedLengthList[0] = 87;
+ *
  *     var growableList = [1, 2];
  *     growableList.length = 0;
  *     growableList.add(499);
  *     growableList[0] = 87;
- *     var unmodifiableList = const [1, 2];
- *     unmodifiableList.length = 0;  // throws.
- *     unmodifiableList.add(499);  // throws
- *     unmodifiableList[0] = 87;  // throws.
  *
  * Lists are [Iterable].
- * List iteration iterates over values in index order.
- * Changing the values will not affect iteration,
- * but changing the valid indices -
- * that is, changing the list's length -
- * between iteration steps
- * will cause a [ConcurrentModificationError].
- * This means that only growable lists can throw [ConcurrentModificationError].
+ * Iteration occurs over values in index order.
+ * Changing the values does not affect iteration,
+ * but changing the valid indices—that is,

[floitsch, 2013/09/23 12:34:46]

not markdown.

+ * changing the list's length—between

[floitsch, 2013/09/23 12:34:46]

ditto.

+ * iteration steps
+ * causes a [ConcurrentModificationError].
+ * This means that only growable lists can throw ConcurrentModificationError.
  * If the length changes temporarily
  * and is restored before continuing the iteration,
- * the iterator will not detect it.
+ * the iterator does not detect it.
  */
 abstract class List<E> implements Iterable<E> {
   /**
-   * Creates a list of the given [length].
+   * Creates a list of the given _length_.
    *
-   * The list is a fixed-length list if [length] is provided, and an empty
-   * growable list if [length] is omitted.
+   * The created list is fixed-length if _length_ is provided.

[floitsch, 2013/09/23 12:34:46]

_length_ is not the same as [length]. Please keep the reference to the argument.
Ditto for all the other occurrences.

[Kathy Walrath, 2013/09/24 00:47:42]

Could we rethink this?

This case is especially bad because the [length] here creates an x-ref to the
length field, not to the length argument.

Also, it's generally distracting to have a bunch of links within text. I'd
rather use plain text except for a few judicious links. In fact, I think
"_length_" can be just "length" or "the length" unless you really need to
clarify that you're talking about the argument.

What do you think, Florian?

[floitsch, 2013/09/24 10:37:14]

That's a bug in dartdoc.
When we created dartdoc we decided not to use Java-like tags (@param, @return,
etc.) but preferred plain English. The idea (at least for me) was to rely on
conventions to get similar behavior. For example, a sentence should start with
"Returns" if it has the same meaning as Java's "@return". Similarly sentences
should start with "Throws", if they describe an exception that is thrown.
Instead of "@param" we (I?) want to highlight sentences that refer to the
argument. So if the editor wants to give context information for a specific
argument it should highlight (or extract) all the sentences that talk about the
argument.
We could (and Lasse just agreed with me) provide even more guidelines: If a
sentence contains "argument [foo]", then this sentence will be used as short
description for the argument. Other occurrences of "[foo]" will be in the long
description.
What this means: one should not just bracket every occurrence of the argument.
In particular for examples it frequently does not make sense:
===
In the following example [x] is set to 0 and therefore blablabla.
===
This sentence should not be part of the short or long description of "x", and
therefore the bracketing of "x" is bad.

So in this case (for the List factory) we could have something like:
Creates a list of the given length.

The optional argument [length] determines the length and type of the list.

The created list is fixed-length if [length] is provided.

The list has length 0 and is growable if [length] is omitted.

[length] must not be negative.


Note that this convention allows to combine the description of several arguments
into one line:
The argument [x] and argument [y] are the coordinates of the point.

Finally: this is mostly a proposal and I don't think there is full agreement on
it.

[Kathy Walrath, 2013/09/24 17:12:41]

It does seem like dartdoc should hew to the closest definition if we want to use
[] in this way. ("Closest definition" isn't equivalent to scope, it seems, since
`length` in the caller's code would refer to the instance variable, not to the
name of the argument.)

So... What if you wanted to refer to the instance variable length in this case?
Would you use [List.length]? E.g.:

The [length] argument, if specified, sets the value of the [List.length]
instance variable.
Can you point me to an example of this?

It would seem to only really work for the first "sentence", since it's a
sentence fragment. (It assumes an elliptical "This method/constructor".) After
the first "sentence", we tend to use real, complete sentences. If we didn't, the
docs would read strangely.
So this would be just for the editor (and other context-sensitive tools), not
necessarily for the API docs? OK, that makes more sense.

I'm still worried that people will leave out the [] or the API docs won't treat
it correctly. And it seems like the API docs are the most important thing, for
now. It's going to be hard/inconvenient/unlikely for comment writers to test how
their comments will work in every permutation. If we could somehow make that
testing easier or more rewarding, that could help.
This is pretty choppy, with all those one-sentence paragraphs. Would it ideally
look more like this?

  The optional argument [length] determines the length and type of the list.
  If you provide [length], the created list is fixed length; if you don't, the
  list has length 0 and is growable. [length] must not be negative.
That sounds awkward to me. If I were writing it naturally, it'd be:

The arguments [x] and [y] are the ....
There's never full agreement. :) But it's good that we're talking.

[floitsch, 2013/09/24 17:21:48]

Not sure. But the argument is clearly in scope (otherwise we wouldn't be able to
refer to them in any dartdoc comment). It doesn't make sense that they come
after the instance variable, since the argument will shadow the instance
variable too.
That's fine. Relax it to: if the sentence contains the word "returns", ...
Well the editor uses the API docs.
That would be great. yes.
Ideally we should start to come up with rules that the editor can then implement
and that the writers can follow. We should have done this a long time ago, but
fortunately changing documentation isn't a breaking change...
Sure.
I guess we could relax the rule to include "," and "and" for the "argument"
case.
Agreed. Maybe I'm completely off (so make sure to talk to others too), but I
always hoped for these rules and support from the editor.

+   * The list has length 0 and is growable if _length_ is omitted.
    *
-   * It is an error if [length] is not a non-negative integer.
+   * An error occurs if _length_ is negative.

[floitsch, 2013/09/23 12:34:46]

This has not the same semantics.
We are saying that it is an error to provide a negative length.
We are not saying that we will check for it.
In this particular example we will probably do it, but still. We prefer "It is
an error".

Ditto for the other occurrences.

[Kathy Walrath, 2013/09/24 00:47:42]

We should definitely be correct about this, but opening a sentence with "It is"
is generally discouraged, because it's unclear (as in this case, where we
misinterpreted its meaning). How about just:

If specified, the length must be 0 or greater.

OR

The length must not be negative.

OR

Do not specify a negative length.

On 2013/09/23 12:34:46, floitsch wrote:

[floitsch, 2013/09/24 10:37:14]

I prefer that one. The problem is, that it becomes inconvenient when the
argument can't have an article. For example:
[isXYZ] must not be `null`.
Then the sentence starts with a lower case.

    */
   external factory List([int length]);
 
   /**
-   * Creates a fixed-length list of the given [length] where each entry
-   * contains [fill].
+   * Creates a fixed-length list of the given _length_
+   * and initializes the value at each position with [fill].
    */
   external factory List.filled(int length, E fill);
 
   /**
-   * Creates an list with the elements of [other].
+   * Creates a list and initializes it using the contents of [other].
    *
-   * The order in the list will be
-   * the order provided by the iterator of [other].
+   * The [Iterator] of [other] provides the order of the objects.
    *
-   * The returned list is growable if [growable] is true, otherwise it's
-   * a fixed length list.
+   * This constructor returns a growable list if [growable] is true;
+   * otherwise, it returns a fixed-length list.
    */
   factory List.from(Iterable other, { bool growable: true }) {
     List<E> list = new List<E>();
@@ -87,14 +81,14 @@ abstract class List<E> implements Iterable<E> {
   }
 
   /**
-   * Generate a `List` of values.
+   * Generates a list of values.
    *
-   * Creates a list with [length] positions
-   * and fills them by values created by calling [generator]
-   * for each index in the range `0` .. `[length] - 1`
+   * Creates a list with _length_ positions
+   * and fills it with values created by calling [generator]
+   * for each index in the range `0` .. `length - 1`
    * in increasing order.
    *
-   * The created list's length is fixed unless [growable] is true.
+   * The created list is fixed-length unless [growable] is true.
    */
   factory List.generate(int length, E generator(int index),
                        { bool growable: true }) {
@@ -111,202 +105,202 @@ abstract class List<E> implements Iterable<E> {
   }
 
   /**
-   * Returns the element at the given [index] in the list or throws
-   * an [RangeError] if [index] is out of bounds.
+   * Returns the object at the given [index] in the list
+   * or throws a [RangeError] if [index] is out of bounds.
    */
   E operator [](int index);
 
   /**
-   * Sets the entry at the given [index] in the list to [value].
-   *
-   * Throws an [RangeError] if [index] is out of bounds.
+   * Sets the value at the given [index] in the list to [value]
+   * or throws a [RangeError] if [index] is out of bounds.
    */
   void operator []=(int index, E value);
 
   /**
-   * Returns the number of elements in the list.
+   * Returns the number of objects in this list.
    *
-   * The valid indices for a list are 0 through `length - 1`.
+   * The valid indices for a list are `0` through `length - 1`.
    */
   int get length;
 
   /**
-   * Changes the length of the list. If [newLength] is greater than
+   * Changes the length of this list.
+   *
+   * If [newLength] is greater than
    * the current [length], entries are initialized to [:null:].
    *
-   * Throws an [UnsupportedError] if the list is not extendable.
+   * Throws an [UnsupportedError] if the list is fixed-length.
    */
   void set length(int newLength);
 
   /**
-   * Adds [value] at the end of the list, extending the length by
-   * one.
+   * Adds [value] to the end of this list,
+   * extending the length by one.
    *
-   * Throws an [UnsupportedError] if the list is not extendable.
+   * Throws an [UnsupportedError] if the list is fixed-length.
    */
   void add(E value);
 
   /**
-   * Appends all elements of the [iterable] to the end of this list.
+   * Appends all objects of [iterable] to the end of this list.
    *
-   * Extends the length of the list by the number of elements in [iterable].
-   * Throws an [UnsupportedError] if this list is not extensible.
+   * Extends the length of the list by the number of objects in [iterable].
+   * Throws an [UnsupportedError] if this list is fixed-length.
    */
   void addAll(Iterable<E> iterable);
 
   /**
-   * Returns an [Iterable] of the elements of this [List] in reverse order.
+   * Returns an [Iterable] of the objects in this list in reverse order.
    */
   Iterable<E> get reversed;
 
   /**
-   * Sorts the list according to the order specified by the [compare] function.
+   * Sorts this list according to the order specified by the [compare] function.
    *
    * The [compare] function must act as a [Comparator].
    *
-   * The default [List] implementations use [Comparable.compare] if
+   * The default List implementations use [Comparable.compare] if
    * [compare] is omitted.
    */
   void sort([int compare(E a, E b)]);
 
   /**
-   * Returns the first index of [element] in the list.
+   * Returns the first index of [element] in this list.
    *
    * Searches the list from index [start] to the length of the list.
-   * The first time an element [:e:] is encountered so that [:e == element:],
-   * the index of [:e:] is returned.
+   * The first time an object [:o:] is encountered so that [:o == element:],
+   * the index of [:o:] is returned.
    * Returns -1 if [element] is not found.
    */
   int indexOf(E element, [int start = 0]);
 
   /**
-   * Returns the last index of [element] in the list.
+   * Returns the last index of [element] in this list.
    *
-   * Searches the list backwards from index [start] (inclusive) to 0.
+   * Searches the list backwards from index [start] to 0.
    *
-   * The first time an element [:e:] is encountered so that [:e == element:],
-   * the index of [:e:] is returned.
+   * The first time an object [:o:] is encountered so that [:o == element:],
+   * the index of [:o:] is returned.
    *
-   * If start is not provided, it defaults to [:this.length - 1:].
+   * If [start] is not provided, it defaults to [:this.length - 1:].
    *
    * Returns -1 if [element] is not found.
    */
   int lastIndexOf(E element, [int start]);
 
   /**
-   * Removes all elements in the list.
+   * Removes all objects from this list;
+   * the length of the list becomes zero.
    *
-   * The length of the list becomes zero.
-   *
-   * Throws an [UnsupportedError], and retains all elements, if the
-   * length of the list cannot be changed.
+   * Throws an [UnsupportedError], and retains all objects, if this 
+   * is a fixed-length list.
    */
   void clear();
 
   /**
-   * Inserts the element at position [index] in the list.
+   * Inserts the object at position [index] in this list.
    *
-   * This increases the length of the list by one and shifts all elements
+   * This increases the length of the list by one and shifts all objects
    * at or after the index towards the end of the list.
    *
-   * It is an error if the [index] does not point inside the list or at the
-   * position after the last element.
+   * An error occurs if the [index] is less than 0 or greater than length.
+   * An [UnsupportedError] occurs if the list is fixed-length.
    */
   void insert(int index, E element);
 
   /**
-   * Inserts all elements of [iterable] at position [index] in the list.
+   * Inserts all objects of [iterable] at position [index] in this list.
    *
    * This increases the length of the list by the length of [iterable] and
-   * shifts all later elements towards the end of the list.
+   * shifts all later objects towards the end of the list.
    *
-   * It is an error if the [index] does not point inside the list or at the
-   * position after the last element.
+   * An error occurs if the [index] is less than 0 or greater than length.
+   * An [UnsupportedError] occurs if the list is fixed-length.
    */
   void insertAll(int index, Iterable<E> iterable);
 
   /**
-   * Overwrites elements of `this` with the elemenst of [iterable] starting
-   * at position [index] in the list.
+   * Overwrites objects of `this` with the objects of [iterable], starting
+   * at position [index] in this list.
    *
    * This operation does not increase the length of `this`.
    *
-   * It is an error if the [index] does not point inside the list or at the
-   * position after the last element.
-   *
-   * It is an error if the [iterable] is longer than [length] - [index].
+   * An error occurs if the [index] is less than 0 or greater than length.
+   * An error occurs if the [iterable] is longer than [length] - [index].
    */
   void setAll(int index, Iterable<E> iterable);
 
   /**
-   * Removes [value] from the list. Returns true if [value] was
-   * in the list. Returns false otherwise. The method has no effect
-   * if [value] value was not in the list.
+   * Removes [value] from this list.
+   *
+   * Returns true if [value] was in the list.
+   * Returns false otherwise.
+   * The method has no effect if [value] was not in the list.
+   *
+   * An [UnsupportedError] occurs if the list is fixed-length.
    */
   bool remove(Object value);
 
   /**
-   * Removes the element at position [index] from the list.
+   * Removes the object at position [index] from this list.
    *
-   * This reduces the length of `this` by one and moves all later elements
+   * This method reduces the length of `this` by one and moves all later objects
    * down by one position.
    *
-   * Returns the removed element.
+   * Returns the removed object.
    *
-   * Throws an [ArgumentError] if [index] is not an [int].
-   *
-   * Throws an [RangeError] if the [index] does not point inside
-   * the list.
-   *
-   * Throws an [UnsupportedError], and doesn't remove the element,
-   * if the length of `this` cannot be changed.
+   * * Throws an [ArgumentError] if [index] is not an [int].
+   * * Throws a [RangeError] if the [index] is out of range for this list.
+   * * Throws an [UnsupportedError], and doesn't remove the object,
+   * if this is a fixed-length list.
    */
   E removeAt(int index);
 
   /**
-   * Pops and returns the last element of the list.
-   * Throws a [UnsupportedError] if the length of the
-   * list cannot be changed.
+   * Pops and returns the last object in this list.
+   *
+   * Throws an [UnsupportedError] if this is a fixed-length list.
    */
   E removeLast();
 
   /**
-   * Removes all elements of this list that satisfy [test].
+   * Removes all objects from this list that satisfy [test].
+   *
+   * An object [:o:] satisfies [test] if [:test(o):] is true.
    *
-   * An elements [:e:] satisfies [test] if [:test(e):] is true.
+   * Throws an [UnsupportedError] if this is a fixed-length list.
    */
   void removeWhere(bool test(E element));
 
   /**
-   * Removes all elements of this list that fail to satisfy [test].
+   * Removes all objects from this list that fail to satisfy [test].
    *
-   * An elements [:e:] satisfies [test] if [:test(e):] is true.
+   * An object [:o:] satisfies [test] if [:test(o):] is true.
+   *
+   * Throws an [UnsupportedError] if this is a fixed-length list.
    */
   void retainWhere(bool test(E element));
 
   /**
-   * Returns a new list containing the elements from [start] to [end].
-   *
-   * The result contains elements of this list with indices greater than or
-   * equal to [start] and less than [end].
+   * Returns a new list containing the objects
+   * from [start] inclusive to [end] exclusive.
    *
    * If [end] is omitted, the [length] of `this` is used.
    *
-   * It is an error if [start] is outside the range `0` .. `[length]` or if
-   * [end] is outside the range `[start]` .. `[length]`.
+   * An error occurs if [start] is outside the range `0` .. `length` or if
+   * [end] is outside the range `start` .. `length`.
    */
   List<E> sublist(int start, [int end]);
 
   /**
-   * Returns an [Iterable] that iterates over the elements in the range
-   * [start] to [end] exclusive. The result of this function
-   * is backed by `this`.
+   * Returns an [Iterable] that iterates over the objects in the range
+   * [start] inclusive to [end] exclusive.
    *
-   * It is an error if [end] is before [start].
+   * An error occurs if [end] is before [start].
    *
-   * It is an error if the [start] and [end] are not valid ranges at the time
-   * of the call to this method. The returned [Iterable] behaves similar to
-   * `skip(start).take(end - start)`. That is, it will not throw exceptions
+   * An error occurs if the [start] and [end] are not valid ranges at the time
+   * of the call to this method. The returned [Iterable] behaves like
+   * `skip(start).take(end - start)`. That is, it does not throw exceptions
    * if `this` changes size.
    *
    * Example:
@@ -320,17 +314,15 @@ abstract class List<E> implements Iterable<E> {
   Iterable<E> getRange(int start, int end);
 
   /**
-   * Copies the elements of [iterable], skipping the [skipCount] first elements,
-   * into the range [start] to [end] exclusive of `this`.
+   * Copies the objects of [iterable], skipping [skipCount] objects first,
+   * into the range [start] inclusive to [end] exclusive of `this`.
    *
    * If [start] equals [end] and [start]..[end] represents a legal range, this
    * method has no effect.
    *
-   * It is an error if [start]..[end] is not a valid range pointing into the
-   * `this`.
-   *
-   * It is an error if the [iterable] does not have enough elements after
-   * skipping [skipCount] elements.
+   * An error occurs if [start]..[end] is not a valid range for `this`.
+   * An error occurs if the [iterable] does not have enough objects after
+   * skipping [skipCount] objects.
    *
    * Example:
    *
@@ -342,28 +334,26 @@ abstract class List<E> implements Iterable<E> {
   void setRange(int start, int end, Iterable<E> iterable, [int skipCount = 0]);
 
   /**
-   * Removes the elements in the range [start] to [end] exclusive.
+   * Removes the objects in the range [start] inclusive to [end] exclusive.
    *
-   * It is an error if [start]..[end] is not a valid range pointing into the
-   * `this`.
+   * An error occurs if [start]..[end] is not a valid range for `this`.
+   * Throws an [UnsupportedError] if this is a fixed-length list.
    */
   void removeRange(int start, int end);
 
   /**
-   * Sets the elements in the range [start] to [end] exclusive to the given
-   * [fillValue].
+   * Sets the objects in the range [start] inclusive to [end] exclusive
+   * to the given [fillValue].
    *
-   * It is an error if [start]..[end] is not a valid range pointing into the
-   * `this`.
+   * An error occurs if [start]..[end] is not a valid range for `this`.
    */
   void fillRange(int start, int end, [E fillValue]);
 
   /**
-   * Removes the elements in the range [start] to [end] exclusive and replaces
-   * them with the contents of the [iterable].
+   * Removes the objects in the range [start] inclusive to [end] exclusive
+   * and replaces them with the contents of the [iterable].
    *
-   * It is an error if [start]..[end] is not a valid range pointing into the
-   * `this`.
+   * An error occurs if [start]..[end] is not a valid range for `this`.
    *
    * Example:
    *
@@ -376,8 +366,8 @@ abstract class List<E> implements Iterable<E> {
   /**
    * Returns an unmodifiable [Map] view of `this`.
    *
-   * It has the indices of this list as keys, and the corresponding elements
-   * as values. The [Map.keys] [Iterable] will iterate the indices of this list
+   * The map uses the indices of this list as keys and the corresponding objects
+   * as values. The `Map.keys` [Iterable] iterates the indices of this list
    * in numerical order.
    */
   Map<int, E> asMap();