Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23462019:
Added examples to map.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 map.dart
R=floitsch@google.com, kathyw@google.com, lrn@google.com
Committed: https://code.google.com/p/dart/source/detail?r=28011
Patch Set 1 #
Total comments: 15
Patch Set 2 : Update based on comments by Mem #
Total comments: 13
Patch Set 3 : Minor post-LGTM changes. #Patch Set 4 : Merge. #Messages
Total messages: 10 (0 generated)
ptal
lovely. A few comments. https://chromiumcodereview.appspot.com/23462019/diff/1/sdk/lib/core/map.dart File sdk/lib/core/map.dart (right): https://chromiumcodereview.appspot.com/23462019/diff/1/sdk/lib/core/map.dart#... sdk/lib/core/map.dart:10: * Please show a code snippet here at the top...minimally creating a Map from a literal. https://chromiumcodereview.appspot.com/23462019/diff/1/sdk/lib/core/map.dart#... sdk/lib/core/map.dart:26: * where the keys and values are computed from the [iterable]. where -> in which https://chromiumcodereview.appspot.com/23462019/diff/1/sdk/lib/core/map.dart#... sdk/lib/core/map.dart:38: * list.map((key) => newMap[key.toString()]).join(', '); // '1, 4, 9, 16' the last line of this code snippet makes this sample too complex. I just want to see the results of the constructor in question (that is, I want to see the what the map looks like with keys and values) and not have to figure out the complexity of the final line https://chromiumcodereview.appspot.com/23462019/diff/1/sdk/lib/core/map.dart#... sdk/lib/core/map.dart:49: * list.map((key) => newMap[key]).join(', '); // '1, 2, 3, 4' same as above. https://chromiumcodereview.appspot.com/23462019/diff/1/sdk/lib/core/map.dart#... sdk/lib/core/map.dart:63: * letters.map((letter) => newMap[letter]).join(', '); // 'axe, ball, cat' same as above. https://chromiumcodereview.appspot.com/23462019/diff/1/sdk/lib/core/map.dart#... sdk/lib/core/map.dart:97: * If [key] is not associated to a value, calls [ifAbsent] and to -> with https://chromiumcodereview.appspot.com/23462019/diff/1/sdk/lib/core/map.dart#... sdk/lib/core/map.dart:107: * scores['Jose']; // 0 where's sophena? Also, use a value that is not 0 to highlight that it's not just defaulting to 0. https://chromiumcodereview.appspot.com/23462019/diff/1/sdk/lib/core/map.dart#... sdk/lib/core/map.dart:110: * [ifAbsent]. An error occurs if you try to add or remove keys...
Another CL that Mem has looked at while you were away. ptal?
https://chromiumcodereview.appspot.com/23462019/diff/1/sdk/lib/core/map.dart File sdk/lib/core/map.dart (right): https://chromiumcodereview.appspot.com/23462019/diff/1/sdk/lib/core/map.dart#... sdk/lib/core/map.dart:26: * where the keys and values are computed from the [iterable]. On 2013/09/06 16:42:22, mem wrote: > where -> in which Done. https://chromiumcodereview.appspot.com/23462019/diff/1/sdk/lib/core/map.dart#... sdk/lib/core/map.dart:38: * list.map((key) => newMap[key.toString()]).join(', '); // '1, 4, 9, 16' On 2013/09/06 16:42:22, mem wrote: > the last line of this code snippet makes this sample too complex. I just want to > see the results of the constructor in question (that is, I want to see the what > the map looks like with keys and values) and not have to figure out the > complexity of the final line Agreed. Simplified. https://chromiumcodereview.appspot.com/23462019/diff/1/sdk/lib/core/map.dart#... sdk/lib/core/map.dart:49: * list.map((key) => newMap[key]).join(', '); // '1, 2, 3, 4' On 2013/09/06 16:42:22, mem wrote: > same as above. Also simplified. https://chromiumcodereview.appspot.com/23462019/diff/1/sdk/lib/core/map.dart#... sdk/lib/core/map.dart:63: * letters.map((letter) => newMap[letter]).join(', '); // 'axe, ball, cat' On 2013/09/06 16:42:22, mem wrote: > same as above. Also simplified. https://chromiumcodereview.appspot.com/23462019/diff/1/sdk/lib/core/map.dart#... sdk/lib/core/map.dart:107: * scores['Jose']; // 0 On 2013/09/06 16:42:22, mem wrote: > where's sophena? > > Also, use a value that is not 0 to highlight that it's not just defaulting to 0. She's there now. Done. https://chromiumcodereview.appspot.com/23462019/diff/1/sdk/lib/core/map.dart#... sdk/lib/core/map.dart:110: * [ifAbsent]. On 2013/09/06 16:42:22, mem wrote: > An error occurs if you try to add or remove keys... Done. https://chromiumcodereview.appspot.com/23462019/diff/1/sdk/lib/core/map.dart#... sdk/lib/core/map.dart:110: * [ifAbsent]. On 2013/09/06 16:42:22, mem wrote: > An error occurs if you try to add or remove keys... Done.
PTAL
LGTM. just nits. https://chromiumcodereview.appspot.com/23462019/diff/7001/sdk/lib/core/map.dart File sdk/lib/core/map.dart (right): https://chromiumcodereview.appspot.com/23462019/diff/7001/sdk/lib/core/map.da... sdk/lib/core/map.dart:43: * If no values are specified for [key] and [value] the default is the or [value] https://chromiumcodereview.appspot.com/23462019/diff/7001/sdk/lib/core/map.da... sdk/lib/core/map.dart:49: * map = new Map.fromIterable(list); Maybe: Since this is not really a useful example: only provide one of the arguments? map = new Map.fromIterable(list, value: (item) => "$item"); map[1]; // "1" ... https://chromiumcodereview.appspot.com/23462019/diff/7001/sdk/lib/core/map.da... sdk/lib/core/map.dart:68: * map['b'] == 'bad' && map['c'] == 'cat'; // true I would prefer: map['b']; // 'bad' map['c']; // 'cat' https://chromiumcodereview.appspot.com/23462019/diff/7001/sdk/lib/core/map.da... sdk/lib/core/map.dart:108: * scores.putIfAbsent(key, () => 25); Maybe put an example that allocates something (or has a side-effect)? To show why this is a function and not just a value. https://chromiumcodereview.appspot.com/23462019/diff/7001/sdk/lib/core/map.da... sdk/lib/core/map.dart:114: * An error occurs if you try to add or remove keys from the map during the We generally write "it is an error". Why do you prefer this wording?
lgtm https://codereview.chromium.org/23462019/diff/7001/sdk/lib/core/map.dart File sdk/lib/core/map.dart (right): https://codereview.chromium.org/23462019/diff/7001/sdk/lib/core/map.dart#newc... sdk/lib/core/map.dart:9: * by using its associated key. Do we want a single-line summary for classes as well as for methods. We generally try to start method descriptions with a single one-line summary. I believe that at some point there was a summary view in the DartDoc that shows only the first line/sentence/something, but I can't find it now. Maybe it's no longer relevant? https://codereview.chromium.org/23462019/diff/7001/sdk/lib/core/map.dart#newc... sdk/lib/core/map.dart:114: * An error occurs if you try to add or remove keys from the map during the I agree with Florian. We write "It is an error" but omit writing what happens if you do it anyway, so that we have some leeway in implementing it. It will *usually* lead to an Error object being thrown, but in unchecked mode the user may sneak in objects that behave in such a way that it completes (with a result you can't trust, ofcourse). This example is a "concurrent modification" error. These are generally "best-effort", and can be cheated by clever users (something like doing 2^32 add/remove operations may make our modification counter wrap and the modification go undetected) - if we specified that it must throw an error, we couldn't even allow this.
lgtm A couple of comments on comments. https://chromiumcodereview.appspot.com/23462019/diff/7001/sdk/lib/core/map.dart File sdk/lib/core/map.dart (right): https://chromiumcodereview.appspot.com/23462019/diff/7001/sdk/lib/core/map.da... sdk/lib/core/map.dart:9: * by using its associated key. On 2013/09/24 08:29:08, Lasse Reichstein Nielsen wrote: > Do we want a single-line summary for classes as well as for methods. Yes, everything needs a single-line summary. > We generally try to start method descriptions with a single one-line summary. I > believe that at some point there was a summary view in the DartDoc that shows > only the first line/sentence/something, but I can't find it now. Maybe it's no > longer relevant? It's in the new dartdoc. You can see it here: http://css-inprogress.dart-documentation.appspot.com/#dart-core#id_Map http://css-inprogress.dart-documentation.appspot.com/#dart-core/Map (It's still a work in progress. Alan's converting it to Polymer, and an external designer is working on the look.) https://chromiumcodereview.appspot.com/23462019/diff/7001/sdk/lib/core/map.da... sdk/lib/core/map.dart:114: * An error occurs if you try to add or remove keys from the map during the I talked with Florian about this on another thread. We try to avoid "It is" because that's a weak way to open a sentence (no content at all, it's anathema to tech writers). Also, "It is an error" makes me think that an error will be thrown, and that's not true. Other ways we could put it would be: Don't add or remove keys in [ifAbsent]. OR The code that [ifAbsent] executes must not add or remove keys.
Message was sent while issue was closed.
Committed patchset #4 manually as r28011 (presubmit successful).
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/23462019/diff/7001/sdk/lib/core/map.dart File sdk/lib/core/map.dart (right): https://chromiumcodereview.appspot.com/23462019/diff/7001/sdk/lib/core/map.da... sdk/lib/core/map.dart:49: * map = new Map.fromIterable(list); On 2013/09/23 12:16:07, floitsch wrote: > Maybe: > Since this is not really a useful example: only provide one of the arguments? > > map = new Map.fromIterable(list, value: (item) => "$item"); > map[1]; // "1" > > ... The point of the example is to show what happens if you provide no arguments. https://chromiumcodereview.appspot.com/23462019/diff/7001/sdk/lib/core/map.da... sdk/lib/core/map.dart:68: * map['b'] == 'bad' && map['c'] == 'cat'; // true On 2013/09/23 12:16:07, floitsch wrote: > I would prefer: > map['b']; // 'bad' > map['c']; // 'cat' I've changed this. I generally try to avoid extremely short expressions. So, for example, the following does not look that good to me: List<int> list = [...]; list.sort(); list; // [...] It is better to make the last line something like this (as long as it doesn't obscure the example): list.join(', '); // ... https://chromiumcodereview.appspot.com/23462019/diff/7001/sdk/lib/core/map.da... sdk/lib/core/map.dart:108: * scores.putIfAbsent(key, () => 25); On 2013/09/23 12:16:07, floitsch wrote: > Maybe put an example that allocates something (or has a side-effect)? > To show why this is a function and not just a value. Done. https://chromiumcodereview.appspot.com/23462019/diff/7001/sdk/lib/core/map.da... sdk/lib/core/map.dart:114: * An error occurs if you try to add or remove keys from the map during the On 2013/09/25 18:11:09, Kathy Walrath wrote: > I talked with Florian about this on another thread. We try to avoid "It is" > because that's a weak way to open a sentence (no content at all, it's anathema > to tech writers). Also, "It is an error" makes me think that an error will be > thrown, and that's not true. Other ways we could put it would be: > > Don't add or remove keys in [ifAbsent]. > > OR > > The code that [ifAbsent] executes must not add or remove keys. I'm using Kathy's version, but I'm not deeply attached to any particular wording. |
