Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23908003:
Added examples to list.dart. (Closed)
|
Created:
7 years, 3 months ago by shailentuli Modified:
7 years, 2 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdded examples to list.dart.
R=floitsch@google.com
Committed: https://code.google.com/p/dart/source/detail?r=28014
Patch Set 1 #
Total comments: 26
Patch Set 2 : Changes based on Mem's comments #
Total comments: 17
Patch Set 3 : Post LGTM tweaks. #Messages
Total messages: 10 (0 generated)
ptal
Nice examples. Mostly just edits. One larger question is do we want exhaustive sets of examples for everything. mem https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart File sdk/lib/core/list.dart (right): https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:48: fixedLengthList.length = 1; // Error Error. With a period. We're using this convention. https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:75: * otherwise, it returns a fixed-length list. no example? ...There are other methods and things below also without examples. What's the thinking about providing examples? Exhaustive (every single thing gets a snippet) or ??? as needed (judgment call by writer?) https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:174: * numbers.sort((x, y) => x.length.compareTo(y.length)); it's a pity sort() doesn't return the list. https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:181: * nums.sort(); add comment-> nums.sort(); // [ -11, 2, 13] https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:187: * Returns the first index of [element] in this list. Technically... it Returns the index of the first occurrence of [element] in this list. https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:189: * Searches the list from index [start] to the length of the list. length -> end https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:195: * notes.indexOf('re', 1); // 1 better if start is 2 and it finds the second one at 3. https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:204: * Returns the last index of [element] in this list. Technically... it Returns the index of the last occurrence of [element] in this list. https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:206: * Searches the list backwards from index [start] to 0. Searches the list backwards for [element] from index [start] to 0. https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:214: * If [start] is not provided, it defaults to [:this.length - 1:]. it defaults ... -> this method searches from the end of the list. https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:240: * An [UnsupportedError] occurs if the list is fixed-length. no example? https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:271: * Removes the first occurence of [value] from this list. occurence -> occurrence prolly should search for other occurrences of occurrence to make sure there are no more occurrences. https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:273: * Returns true if [value] was in the list. ... in the list, false otherwise. then delete "Returns false otherwise." https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:277: * parts.remove('head'); // true oh no! a decapitation! https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:283: * parts.remove('head'); // false maybe use 'elbow' here and you can remove the "note:" https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:361: * of the call to this method. The returned [Iterable] behaves like The last sentence here illuminates the example, but is buried under all of the info about errors. Put a paragraph break between "method." and "The returned ..." and move the error business below the sample. https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:380: * // list1 Orphan! Bad form to put one word on a line by itself.
Can you take a look, Kathy?
Can you take a look, Kathy?
https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart File sdk/lib/core/list.dart (right): https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:48: fixedLengthList.length = 1; // Error On 2013/09/06 16:28:16, mem wrote: > Error. > > With a period. We're using this convention. Done. https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:174: * numbers.sort((x, y) => x.length.compareTo(y.length)); On 2013/09/06 16:28:16, mem wrote: > it's a pity sort() doesn't return the list. Yup, it's an in-place sort. https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:181: * nums.sort(); On 2013/09/06 16:28:16, mem wrote: > add comment-> > nums.sort(); // [ -11, 2, 13] This goes against our convention that the // signifies the value returned by whatever's on the left hand side. The next line shows the result. https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:195: * notes.indexOf('re', 1); // 1 On 2013/09/06 16:28:16, mem wrote: > better if start is 2 and it finds the second one at 3. Done. https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:214: * If [start] is not provided, it defaults to [:this.length - 1:]. On 2013/09/06 16:28:16, mem wrote: > it defaults ... -> this method searches from the end of the list. Done. https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:273: * Returns true if [value] was in the list. On 2013/09/06 16:28:16, mem wrote: > ... in the list, false otherwise. > > then delete "Returns false otherwise." Done. https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:277: * parts.remove('head'); // true On 2013/09/06 16:28:16, mem wrote: > oh no! a decapitation! oh yes! https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:283: * parts.remove('head'); // false On 2013/09/06 16:28:16, mem wrote: > maybe use 'elbow' here and you can remove the "note:" No, it has to be 'head' to show that `remove()` returns false if it does not actually remove anything. https://chromiumcodereview.appspot.com/23908003/diff/1/sdk/lib/core/list.dart... sdk/lib/core/list.dart:380: * // list1 On 2013/09/06 16:28:16, mem wrote: > Orphan! Bad form to put one word on a line by itself. Done.
PTAL
LGTM. mostly comments for older changes (from https://codereview.chromium.org/23190016/) https://chromiumcodereview.appspot.com/23908003/diff/8001/sdk/lib/core/list.dart File sdk/lib/core/list.dart (right): https://chromiumcodereview.appspot.com/23908003/diff/8001/sdk/lib/core/list.d... sdk/lib/core/list.dart:26: end spurious line. https://chromiumcodereview.appspot.com/23908003/diff/8001/sdk/lib/core/list.d... sdk/lib/core/list.dart:34: * indices—that is, changing the list's length—between iteration — is not valid markdown (afaik). (twice on this line). https://chromiumcodereview.appspot.com/23908003/diff/8001/sdk/lib/core/list.d... sdk/lib/core/list.dart:36: * lists can throw ConcurrentModificationError. If the length changes [ConcurrentModificationError] or `ConcurrentModificationError` or concurrent-modification errors. https://chromiumcodereview.appspot.com/23908003/diff/8001/sdk/lib/core/list.d... sdk/lib/core/list.dart:42: * Creates a list of the given _length_. [length] https://chromiumcodereview.appspot.com/23908003/diff/8001/sdk/lib/core/list.d... sdk/lib/core/list.dart:44: * The created list is fixed-length if _length_ is provided. [length] https://chromiumcodereview.appspot.com/23908003/diff/8001/sdk/lib/core/list.d... sdk/lib/core/list.dart:51: * The list has length 0 and is growable if _length_ is omitted. [length] https://chromiumcodereview.appspot.com/23908003/diff/8001/sdk/lib/core/list.d... sdk/lib/core/list.dart:57: * An error occurs if _length_ is negative. I would prefer: The argument [length] must not be negative. https://chromiumcodereview.appspot.com/23908003/diff/8001/sdk/lib/core/list.d... sdk/lib/core/list.dart:62: * Creates a fixed-length list of the given _length_ [length] https://chromiumcodereview.appspot.com/23908003/diff/8001/sdk/lib/core/list.d... sdk/lib/core/list.dart:174: * numbers.sort((x, y) => x.length.compareTo(y.length)); you could also show that short-cutting is ok: numbers.sort((x, y) => x.length - y.length); https://chromiumcodereview.appspot.com/23908003/diff/8001/sdk/lib/core/list.d... sdk/lib/core/list.dart:183: */ Since you add an example that sorts strings, please add: The `compareTo` function of strings is locale unaware and yields wrong results in many languages. -- Unfortunately we don't have support for collators yet. (post v1). https://chromiumcodereview.appspot.com/23908003/diff/8001/sdk/lib/core/list.d... sdk/lib/core/list.dart:199: * notes.indexOf('fa'); // -1 either align with above, or put it closer to the statement. https://chromiumcodereview.appspot.com/23908003/diff/8001/sdk/lib/core/list.d... sdk/lib/core/list.dart:215: * list./Returns Spurious text. https://chromiumcodereview.appspot.com/23908003/diff/8001/sdk/lib/core/list.d... sdk/lib/core/list.dart:286: * An [UnsupportedError] occurs if the list is fixed-length. Below we write "Throws an [UnsupportedError] if this is a fixed-length list." I prefer the wording below.
A couple of comments on comments. https://chromiumcodereview.appspot.com/23908003/diff/8001/sdk/lib/core/list.dart File sdk/lib/core/list.dart (right): https://chromiumcodereview.appspot.com/23908003/diff/8001/sdk/lib/core/list.d... sdk/lib/core/list.dart:36: * lists can throw ConcurrentModificationError. If the length changes On 2013/09/23 13:41:23, floitsch wrote: > [ConcurrentModificationError] or `ConcurrentModificationError` or > concurrent-modification errors. Why not just ConcurrentModificationError or ConcurrentModificationErrors? Here's my reasoning: - There's no risk of mistaking this word for ordinary English. - "[...]" adds a link, which is unnecessary since there's a link in the previous sentence. - "`...`" adds unnecessary and ugly code font. - "concurrent-modification errors" is hard to read, and you'll just parse it back into ConcurrentModificationErrors. https://chromiumcodereview.appspot.com/23908003/diff/8001/sdk/lib/core/list.d... sdk/lib/core/list.dart:42: * Creates a list of the given _length_. On 2013/09/23 13:41:23, floitsch wrote: > [length] How about just "of the given length." (I think we discussed this in another CL.)
https://chromiumcodereview.appspot.com/23908003/diff/8001/sdk/lib/core/list.dart File sdk/lib/core/list.dart (right): https://chromiumcodereview.appspot.com/23908003/diff/8001/sdk/lib/core/list.d... sdk/lib/core/list.dart:36: * lists can throw ConcurrentModificationError. If the length changes On 2013/09/25 18:26:25, Kathy Walrath wrote: > On 2013/09/23 13:41:23, floitsch wrote: > > [ConcurrentModificationError] or `ConcurrentModificationError` or > > concurrent-modification errors. > > Why not just ConcurrentModificationError or ConcurrentModificationErrors? Here's > my reasoning: > - There's no risk of mistaking this word for ordinary English. > - "[...]" adds a link, which is unnecessary since there's a link in the previous > sentence. But it also means that renaming the class will automatically rename this comment. > - "`...`" adds unnecessary and ugly code font. But ConcurrentModificationError is code. It's not English. > - "concurrent-modification errors" is hard to read, and you'll just parse it > back into ConcurrentModificationErrors. In the end I don't have a strong opinion on this. I can easily live with the original text. https://chromiumcodereview.appspot.com/23908003/diff/8001/sdk/lib/core/list.d... sdk/lib/core/list.dart:42: * Creates a list of the given _length_. On 2013/09/25 18:26:25, Kathy Walrath wrote: > On 2013/09/23 13:41:23, floitsch wrote: > > [length] > > How about just "of the given length." > > (I think we discussed this in another CL.) fine.
Message was sent while issue was closed.
Committed patchset #3 manually as r28014 (presubmit successful). |
