Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23190016:
editing doc comments for List class (Closed)
|
Created:
7 years, 4 months ago by mem Modified:
7 years, 3 months ago CC:
Lasse Reichstein Nielsen Visibility:
Public. |
Descriptionediting doc comments for List class
BUG=
R=kathyw@google.com
Committed: https://code.google.com/p/dart/source/detail?r=26434
Patch Set 1 #
Total comments: 54
Patch Set 2 : integrated KW and Seth's comments #
Total comments: 8
Patch Set 3 : tweaks according to KW #Patch Set 4 : removed/corrected a couple of links #Patch Set 5 : merging with master & final tweaks #
Total comments: 10
Messages
Total messages: 13 (0 generated)
Editing pass on List class.
more to come https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart File sdk/lib/core/list.dart (right): https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcode19 sdk/lib/core/list.dart:19: * * Unmodifiable list. There's also http://api.dartlang.org/docs/releases/latest/unmodifiable_collection/NonGrowa... I think it's OK to omit unmodifiable list from this description here. From this class'es perspective, there's growable and there's fixed length.
lots o' nits https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart File sdk/lib/core/list.dart (right): https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcode28 sdk/lib/core/list.dart:28: * Examples: Add some more words here. I wasn't sure what these examples were about. Maybe combine the two paragraphs. E.g.: The following code illustrates that some List implementations support only a subset of the API: https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcode40 sdk/lib/core/list.dart:40: * var unmodifiableList = const [1, 2]; delete unmodifiableList example if you remove it from the list o' lists above. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcode79 sdk/lib/core/list.dart:79: * This constructor returns a growable list if [growable] is true, true, -> true; https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcode99 sdk/lib/core/list.dart:99: * Creates a list with _length_ positions shouldn't _length_ be [length]? We should figure out the convention... https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:104: * The length of the created list is fixed unless [growable] is true. Use "fixed-length" here? https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:140: * Changes the length of the list. If [newLength] is greater than add blank line before "If" https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:151: * Throws an [UnsupportedError] if the list is not growable. the list is not growable -> this is a fixed-length list https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:159: * Throws an [UnsupportedError] if this list is not growable. see comment for add() https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:207: * length of the list cannot be changed. the length... -> this is a fixed-length list https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:217: * An error occurs if the [index] is less than 0 or greater than length. isn't there an UnsupportedException if this is a fixed-length list? https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:227: * An error occurs if the [index] is less than 0 or greater than length. isn't there an UnsupportedException if this is a fixed-length list? https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:232: * Overwrites objects of `this` with the objects of [iterable] starting starting -> , starting https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:237: * An error occurs if the [index] is less than 0 or greater than length. It seems like both errors could go in a single paragraph or be in a bulleted list. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:244: * Removes [value] from the list. Returns true if [value] was convert to one-sentence first paragraph. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:253: * This reduces the length of `this` by one and moves all later objects This -> This method https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:258: * Throws an [ArgumentError] if [index] is not an [int]. It seems excessive to have a paragraph per "Throws". Use a bulleted list or single paragraph instead. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:263: * if the length of `this` cannot be changed. See the "fixed-length" comment for removeLast(). https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:268: * Pops and returns the last object of the list. add blank line after this one https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:269: * Throws a [UnsupportedError] if the length of the a -> an how about "if the length..." -> "this is a fixed-length list" https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:277: * An object [:o:] satisfies [test] if [:test(o):] is true. Probably throws an UnsupportedError if this is a fixed-length list, no? https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:284: * An object [:o:] satisfies [test] if [:test(o):] is true. Probably throws an UnsupportedError if this is a fixed-length list, no? https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:301: * [start] inclusive to [end] exclusive. The result of this function Add blank line before "The result" https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:302: * is backed by `this`. I don't know what this means. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:307: * of the call to this method. The returned [Iterable] behaves similar to similar to -> similarly to OR -> like https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:308: * `skip(start).take(end - start)`. That is, it will not throw exceptions will -> does https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:345: * An error occurs if [start]..[end] is not a valid range for `this`. isn't there an UnsupportedException if this is a fixed-length list?
https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart File sdk/lib/core/list.dart (right): https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcode19 sdk/lib/core/list.dart:19: * * Unmodifiable list. On 2013/08/19 15:59:28, sethladd wrote: > There's also > http://api.dartlang.org/docs/releases/latest/unmodifiable_collection/NonGrowa... > > I think it's OK to omit unmodifiable list from this description here. From this > class'es perspective, there's growable and there's fixed length. Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcode28 sdk/lib/core/list.dart:28: * Examples: On 2013/08/19 22:41:10, Kathy Walrath wrote: > Add some more words here. I wasn't sure what these examples were about. Maybe > combine the two paragraphs. E.g.: > > The following code illustrates > that some List implementations > support only a subset of the API: Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcode40 sdk/lib/core/list.dart:40: * var unmodifiableList = const [1, 2]; On 2013/08/19 22:41:10, Kathy Walrath wrote: > delete unmodifiableList example if you remove it from the list o' lists above. Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcode79 sdk/lib/core/list.dart:79: * This constructor returns a growable list if [growable] is true, On 2013/08/19 22:41:10, Kathy Walrath wrote: > true, -> true; Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcode99 sdk/lib/core/list.dart:99: * Creates a list with _length_ positions The word length in this context is overloaded. Here it is both the parameter name and the name of a property in this class. I didn't want it to link to the property so I used _ On 2013/08/19 22:41:10, Kathy Walrath wrote: > shouldn't _length_ be [length]? We should figure out the convention... https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:104: * The length of the created list is fixed unless [growable] is true. On 2013/08/19 22:41:10, Kathy Walrath wrote: > Use "fixed-length" here? Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:140: * Changes the length of the list. If [newLength] is greater than On 2013/08/19 22:41:10, Kathy Walrath wrote: > add blank line before "If" Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:151: * Throws an [UnsupportedError] if the list is not growable. On 2013/08/19 22:41:10, Kathy Walrath wrote: > the list is not growable -> this is a fixed-length list Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:159: * Throws an [UnsupportedError] if this list is not growable. On 2013/08/19 22:41:10, Kathy Walrath wrote: > see comment for add() Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:207: * length of the list cannot be changed. On 2013/08/19 22:41:10, Kathy Walrath wrote: > the length... -> this is a fixed-length list Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:217: * An error occurs if the [index] is less than 0 or greater than length. Why yes there is. UnsupportedError. On 2013/08/19 22:41:10, Kathy Walrath wrote: > isn't there an UnsupportedException if this is a fixed-length list? https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:227: * An error occurs if the [index] is less than 0 or greater than length. On 2013/08/19 22:41:10, Kathy Walrath wrote: > isn't there an UnsupportedException if this is a fixed-length list? Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:232: * Overwrites objects of `this` with the objects of [iterable] starting On 2013/08/19 22:41:10, Kathy Walrath wrote: > starting -> , starting Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:237: * An error occurs if the [index] is less than 0 or greater than length. On 2013/08/19 22:41:10, Kathy Walrath wrote: > It seems like both errors could go in a single paragraph or be in a bulleted > list. Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:244: * Removes [value] from the list. Returns true if [value] was On 2013/08/19 22:41:10, Kathy Walrath wrote: > convert to one-sentence first paragraph. Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:253: * This reduces the length of `this` by one and moves all later objects On 2013/08/19 22:41:10, Kathy Walrath wrote: > This -> This method Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:258: * Throws an [ArgumentError] if [index] is not an [int]. On 2013/08/19 22:41:10, Kathy Walrath wrote: > It seems excessive to have a paragraph per "Throws". Use a bulleted list or > single paragraph instead. Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:263: * if the length of `this` cannot be changed. On 2013/08/19 22:41:10, Kathy Walrath wrote: > See the "fixed-length" comment for removeLast(). Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:268: * Pops and returns the last object of the list. On 2013/08/19 22:41:10, Kathy Walrath wrote: > add blank line after this one Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:269: * Throws a [UnsupportedError] if the length of the On 2013/08/19 22:41:10, Kathy Walrath wrote: > a -> an > > how about "if the length..." -> "this is a fixed-length list" Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:277: * An object [:o:] satisfies [test] if [:test(o):] is true. On 2013/08/19 22:41:10, Kathy Walrath wrote: > Probably throws an UnsupportedError if this is a fixed-length list, no? Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:284: * An object [:o:] satisfies [test] if [:test(o):] is true. On 2013/08/19 22:41:10, Kathy Walrath wrote: > Probably throws an UnsupportedError if this is a fixed-length list, no? Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:301: * [start] inclusive to [end] exclusive. The result of this function On 2013/08/19 22:41:10, Kathy Walrath wrote: > Add blank line before "The result" Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:302: * is backed by `this`. On 2013/08/19 22:41:10, Kathy Walrath wrote: > I don't know what this means. Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:307: * of the call to this method. The returned [Iterable] behaves similar to On 2013/08/19 22:41:10, Kathy Walrath wrote: > similar to -> similarly to > OR > -> like Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:308: * `skip(start).take(end - start)`. That is, it will not throw exceptions On 2013/08/19 22:41:10, Kathy Walrath wrote: > will -> does Done. https://codereview.chromium.org/23190016/diff/1/sdk/lib/core/list.dart#newcod... sdk/lib/core/list.dart:345: * An error occurs if [start]..[end] is not a valid range for `this`. On 2013/08/19 22:41:10, Kathy Walrath wrote: > isn't there an UnsupportedException if this is a fixed-length list? Done.
A few quibbles. Otherwise, LGTM. https://codereview.chromium.org/23190016/diff/6001/sdk/lib/core/list.dart File sdk/lib/core/list.dart (right): https://codereview.chromium.org/23190016/diff/6001/sdk/lib/core/list.dart#new... sdk/lib/core/list.dart:91: * The created list is of fixed-length unless [growable] is true. Technically, it should be "fixed length" (no hyphen) unless it precedes a noun. So "fixed-length list" but "list of fixed length". Actually, "list is of fixed length" sounds overly formal to me. Drop the "of", keep the hyphen, and I'll look the other way. I considered using non-growable (though not here), so people could just search for "growable". But "fixed[-]length" is more understandable, I think. https://codereview.chromium.org/23190016/diff/6001/sdk/lib/core/list.dart#new... sdk/lib/core/list.dart:137: * Adds [value] to the end of the list, the list -> this list [global] https://codereview.chromium.org/23190016/diff/6001/sdk/lib/core/list.dart#new... sdk/lib/core/list.dart:153: * Returns an [Iterable] of the objects in this List in reverse order. this List -> this list https://codereview.chromium.org/23190016/diff/6001/sdk/lib/core/list.dart#new... sdk/lib/core/list.dart:238: * The method has no effect if [value] was not in the list. No UnsupportedError?
https://codereview.chromium.org/23190016/diff/6001/sdk/lib/core/list.dart File sdk/lib/core/list.dart (right): https://codereview.chromium.org/23190016/diff/6001/sdk/lib/core/list.dart#new... sdk/lib/core/list.dart:91: * The created list is of fixed-length unless [growable] is true. On 2013/08/20 21:39:34, Kathy Walrath wrote: > Technically, it should be "fixed length" (no hyphen) unless it precedes a noun. > So "fixed-length list" but "list of fixed length". > > Actually, "list is of fixed length" sounds overly formal to me. Drop the "of", > keep the hyphen, and I'll look the other way. > > I considered using non-growable (though not here), so people could just search > for "growable". But "fixed[-]length" is more understandable, I think. Done. https://codereview.chromium.org/23190016/diff/6001/sdk/lib/core/list.dart#new... sdk/lib/core/list.dart:137: * Adds [value] to the end of the list, On 2013/08/20 21:39:34, Kathy Walrath wrote: > the list -> this list > > [global] Done. https://codereview.chromium.org/23190016/diff/6001/sdk/lib/core/list.dart#new... sdk/lib/core/list.dart:153: * Returns an [Iterable] of the objects in this List in reverse order. On 2013/08/20 21:39:34, Kathy Walrath wrote: > this List -> this list Done. https://codereview.chromium.org/23190016/diff/6001/sdk/lib/core/list.dart#new... sdk/lib/core/list.dart:238: * The method has no effect if [value] was not in the list. On 2013/08/20 21:39:34, Kathy Walrath wrote: > No UnsupportedError? Done.
LGTM. The only remaining thing I noticed (as we talked about off this thread) was removing unnecessary [link]s from the descriptions—in particular, from the first paragraph.
Message was sent while issue was closed.
Committed patchset #5 manually as r26434 (presubmit successful).
Message was sent while issue was closed.
DBC: I already made the same comments on Shailen's CL [0], but just to have them in the correct place. Also, please CC one of us when doing changes to the core documentation. [0] https://chromiumcodereview.appspot.com/23908003/ https://codereview.chromium.org/23190016/diff/18001/sdk/lib/core/list.dart File sdk/lib/core/list.dart (right): https://codereview.chromium.org/23190016/diff/18001/sdk/lib/core/list.dart#ne... sdk/lib/core/list.dart:35: * but changing the valid indices—that is, not markdown. https://codereview.chromium.org/23190016/diff/18001/sdk/lib/core/list.dart#ne... sdk/lib/core/list.dart:36: * changing the list's length—between ditto. https://codereview.chromium.org/23190016/diff/18001/sdk/lib/core/list.dart#ne... sdk/lib/core/list.dart:48: * The created list is fixed-length if _length_ is provided. _length_ is not the same as [length]. Please keep the reference to the argument. Ditto for all the other occurrences. https://codereview.chromium.org/23190016/diff/18001/sdk/lib/core/list.dart#ne... sdk/lib/core/list.dart:51: * An error occurs if _length_ is negative. 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.
Message was sent while issue was closed.
A few comments on Florian's comments. https://codereview.chromium.org/23190016/diff/18001/sdk/lib/core/list.dart File sdk/lib/core/list.dart (right): https://codereview.chromium.org/23190016/diff/18001/sdk/lib/core/list.dart#ne... sdk/lib/core/list.dart:48: * The created list is fixed-length if _length_ is provided. On 2013/09/23 12:34:46, floitsch wrote: > _length_ is not the same as [length]. Please keep the reference to the argument. > Ditto for all the other occurrences. 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? https://codereview.chromium.org/23190016/diff/18001/sdk/lib/core/list.dart#ne... sdk/lib/core/list.dart:51: * An error occurs if _length_ is negative. 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: > 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.
Message was sent while issue was closed.
https://codereview.chromium.org/23190016/diff/18001/sdk/lib/core/list.dart File sdk/lib/core/list.dart (right): https://codereview.chromium.org/23190016/diff/18001/sdk/lib/core/list.dart#ne... sdk/lib/core/list.dart:48: * The created list is fixed-length if _length_ is provided. On 2013/09/24 00:47:42, Kathy Walrath wrote: > On 2013/09/23 12:34:46, floitsch wrote: > > _length_ is not the same as [length]. Please keep the reference to the > argument. > > Ditto for all the other occurrences. > > 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. That's a bug in dartdoc. > > 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? 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. https://codereview.chromium.org/23190016/diff/18001/sdk/lib/core/list.dart#ne... sdk/lib/core/list.dart:51: * An error occurs if _length_ is negative. On 2013/09/24 00:47:42, Kathy Walrath wrote: > 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. 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. > > OR > > Do not specify a negative length. > > On 2013/09/23 12:34:46, floitsch wrote: > > 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. >
Message was sent while issue was closed.
https://codereview.chromium.org/23190016/diff/18001/sdk/lib/core/list.dart File sdk/lib/core/list.dart (right): https://codereview.chromium.org/23190016/diff/18001/sdk/lib/core/list.dart#ne... sdk/lib/core/list.dart:48: * The created list is fixed-length if _length_ is provided. > > This case is especially bad because the [length] here creates an x-ref to the > > length field, not to the length argument. > That's a bug in dartdoc. 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. > > 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? > > 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. 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. > 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. 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. > 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. 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. > 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. That sounds awkward to me. If I were writing it naturally, it'd be: The arguments [x] and [y] are the .... > Finally: this is mostly a proposal and I don't think there is full agreement on > it. There's never full agreement. :) But it's good that we're talking.
Message was sent while issue was closed.
https://codereview.chromium.org/23190016/diff/18001/sdk/lib/core/list.dart File sdk/lib/core/list.dart (right): https://codereview.chromium.org/23190016/diff/18001/sdk/lib/core/list.dart#ne... sdk/lib/core/list.dart:48: * The created list is fixed-length if _length_ is provided. On 2013/09/24 17:12:41, Kathy Walrath wrote: > > > This case is especially bad because the [length] here creates an x-ref to > the > > > length field, not to the length argument. > > That's a bug in dartdoc. > > 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. 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. > > > > 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? > > > > 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. > > 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. That's fine. Relax it to: if the sentence contains the word "returns", ... > > > 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. > > So this would be just for the editor (and other context-sensitive tools), not > necessarily for the API docs? OK, that makes more sense. Well the editor uses the API docs. > > 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. 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... > > > 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. > > 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. Sure. > > > 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. > > That sounds awkward to me. If I were writing it naturally, it'd be: > > The arguments [x] and [y] are the .... I guess we could relax the rule to include "," and "and" for the "argument" case. > > > Finally: this is mostly a proposal and I don't think there is full agreement > on > > it. > > There's never full agreement. :) But it's good that we're talking. 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. |
