Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9599015:
documented component (Closed)
|
Created:
8 years, 9 months ago by mattsh Modified:
8 years, 9 months ago CC:
reviews_dartlang.org Visibility:
Public. |
Descriptiondocumented component
BUG=
TEST=
Committed: https://code.google.com/p/dart/source/detail?r=4968
Patch Set 1 #
Total comments: 26
Patch Set 2 : removed blank lines #Patch Set 3 : code review updates #
Total comments: 3
Patch Set 4 : added bullets #Patch Set 5 : moved documentation of component to test_options.dart #
Total comments: 2
Patch Set 6 : fix flag description #Messages
Total messages: 10 (0 generated)
Added documentation for "component" flag of dart checkin suite.
https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart File tools/test.dart (right): https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode7 tools/test.dart:7: * This file is the entrypoint of the dart checkin suite. This checkin I never heard the term 'checkin suite' before, I'm not sure what it means. I was expecting you'd say 'test suite'. https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode11 tools/test.dart:11: * 2. the frog compiler (compiles dart to js) maybe mention that we ensure the gen code runs correctly in multiple browsers (DumpRenderTree, chrome, FF, ie) https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode16 tools/test.dart:16: * io libraries etc.) weird indent here. Maybe align with 'other'? 6. other ... io libraries... https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode22 tools/test.dart:22: * are documented in [tools/testing/test_options.dart] I believe the notation [something] is only to point to symbols in the code (a variable, method, class), but not files. Maybe change to [TestOptionParser] or use quotes. https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode24 tools/test.dart:24: * The "component" argument is complicated and is documented here. I'd change this a bit - maybe reorder by saying: Test.py takes many command line arguments. The "component" argument defines which part of the system is being tested and how. Here is a detail description for what each component means: * vm: ... ... All other arguments are straightforward and documented in test_options.dart https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode28 tools/test.dart:28: * vm: run dart code on the standalone dart vm (nit): make this also bulleted, fix indentation also here (I might prefer here using alignment rather than +4) - vm: ... - frog: compile ... run the resulting ... - leg: compile ... run ... - ... https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode35 tools/test.dart:35: * dartium: run dart code in dartium actually we run it in a dartium-DumpRenderTree (a headless version of dartium) https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode38 tools/test.dart:38: * and run the resulting javascript in DumpRenderTree DumpRenderTree (a headless version of chromium) https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode45 tools/test.dart:45: * dart code add closing paren https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode47 tools/test.dart:47: * remove extra lines https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode50 tools/test.dart:50: here too
Thanks for quick review. OK, fixed up per comments. https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart File tools/test.dart (right): https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode7 tools/test.dart:7: * This file is the entrypoint of the dart checkin suite. This checkin On 2012/03/05 17:10:45, sigmund wrote: > I never heard the term 'checkin suite' before, I'm not sure what it means. I was > expecting you'd say 'test suite'. Done. https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode11 tools/test.dart:11: * 2. the frog compiler (compiles dart to js) On 2012/03/05 17:10:45, sigmund wrote: > maybe mention that we ensure the gen code runs correctly in multiple browsers > (DumpRenderTree, chrome, FF, ie) I just wanted this list here to be a very quick summary of what is being tested, so I don't think that quite fits here. https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode16 tools/test.dart:16: * io libraries etc.) On 2012/03/05 17:10:45, sigmund wrote: > weird indent here. Maybe align with 'other'? > 6. other ... > io libraries... Just the normal 4 space line continuation indent. (I didn't want to align with "other" since then it looks like it might be a distinct item in the list but missing its number.) https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode22 tools/test.dart:22: * are documented in [tools/testing/test_options.dart] On 2012/03/05 17:10:45, sigmund wrote: > I believe the notation [something] is only to point to symbols in the code (a > variable, method, class), but not files. Maybe change to [TestOptionParser] or > use quotes. OK, changed to quotes now. https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode24 tools/test.dart:24: * The "component" argument is complicated and is documented here. On 2012/03/05 17:10:45, sigmund wrote: > I'd change this a bit - maybe reorder by saying: > > Test.py takes many command line arguments. The "component" argument defines > which part of the system is being tested and how. Here is a detail description > for what each component means: > * vm: ... > ... > > All other arguments are straightforward and documented in test_options.dart > OK, reworded and shortened this text. https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode28 tools/test.dart:28: * vm: run dart code on the standalone dart vm On 2012/03/05 17:10:45, sigmund wrote: > (nit): make this also bulleted, fix indentation also here (I might prefer here > using alignment rather than +4) > > - vm: ... > - frog: compile ... > run the resulting ... > - leg: compile ... > run ... > - ... slightly prefer to keep as is https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode35 tools/test.dart:35: * dartium: run dart code in dartium On 2012/03/05 17:10:45, sigmund wrote: > actually we run it in a dartium-DumpRenderTree (a headless version of dartium) OK, thanks (siggi and I just chatted). Updated comment to explain this. https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode38 tools/test.dart:38: * and run the resulting javascript in DumpRenderTree On 2012/03/05 17:10:45, sigmund wrote: > DumpRenderTree (a headless version of chromium) Done. https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode45 tools/test.dart:45: * dart code On 2012/03/05 17:10:45, sigmund wrote: > add closing paren Done. https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode47 tools/test.dart:47: * On 2012/03/05 17:10:45, sigmund wrote: > remove extra lines Done. https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode50 tools/test.dart:50: On 2012/03/05 17:10:45, sigmund wrote: > here too Done.
https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart File tools/test.dart (right): https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode16 tools/test.dart:16: * io libraries etc.) On 2012/03/05 17:49:09, mattsh wrote: > On 2012/03/05 17:10:45, sigmund wrote: > > weird indent here. Maybe align with 'other'? > > 6. other ... > > io libraries... > > Just the normal 4 space line continuation indent. (I didn't want to align with > "other" since then it looks like it might be a distinct item in the list but > missing its number.) I was expecting expecting +4 from the bullet, not from the start of the text. https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode28 tools/test.dart:28: * vm: run dart code on the standalone dart vm On 2012/03/05 17:49:09, mattsh wrote: > On 2012/03/05 17:10:45, sigmund wrote: > > (nit): make this also bulleted, fix indentation also here (I might prefer here > > using alignment rather than +4) > > > > - vm: ... > > - frog: compile ... > > run the resulting ... > > - leg: compile ... > > run ... > > - ... > > slightly prefer to keep as is please consider adding the bullet, I believe if we run this file by dartdoc, it will be formatted better with them. https://chromiumcodereview.appspot.com/9599015/diff/5001/tools/test.dart File tools/test.dart (right): https://chromiumcodereview.appspot.com/9599015/diff/5001/tools/test.dart#newc... tools/test.dart:24: * The the "component" command line arg can be: delete 'the'
https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart File tools/test.dart (right): https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode16 tools/test.dart:16: * io libraries etc.) On 2012/03/05 17:54:25, sigmund wrote: > On 2012/03/05 17:49:09, mattsh wrote: > > On 2012/03/05 17:10:45, sigmund wrote: > > > weird indent here. Maybe align with 'other'? > > > 6. other ... > > > io libraries... > > > > Just the normal 4 space line continuation indent. (I didn't want to align > with > > "other" since then it looks like it might be a distinct item in the list but > > missing its number.) > > I was expecting expecting +4 from the bullet, not from the start of the text. OK, but +4 from the bullet will put it just one space in from the first word, which looks a bit strange. https://chromiumcodereview.appspot.com/9599015/diff/1/tools/test.dart#newcode28 tools/test.dart:28: * vm: run dart code on the standalone dart vm On 2012/03/05 17:54:25, sigmund wrote: > On 2012/03/05 17:49:09, mattsh wrote: > > On 2012/03/05 17:10:45, sigmund wrote: > > > (nit): make this also bulleted, fix indentation also here (I might prefer > here > > > using alignment rather than +4) > > > > > > - vm: ... > > > - frog: compile ... > > > run the resulting ... > > > - leg: compile ... > > > run ... > > > - ... > > > > slightly prefer to keep as is > > please consider adding the bullet, I believe if we run this file by dartdoc, it > will be formatted better with them. Ah, good point. I didn't know dartdoc picks up the bullets, ok added them.
DBC https://chromiumcodereview.appspot.com/9599015/diff/5001/tools/test.dart File tools/test.dart (right): https://chromiumcodereview.appspot.com/9599015/diff/5001/tools/test.dart#newc... tools/test.dart:26: * vm: run dart code on the standalone dart vm Instead of a comment, how about moving it to TestOptionsParser in dart/tools/testing/dart/test_options.dart. Instead of the string 'The component to test against', print the contents of this comment.
https://chromiumcodereview.appspot.com/9599015/diff/5001/tools/test.dart File tools/test.dart (right): https://chromiumcodereview.appspot.com/9599015/diff/5001/tools/test.dart#newc... tools/test.dart:26: * vm: run dart code on the standalone dart vm On 2012/03/05 18:00:46, ahe wrote: > Instead of a comment, how about moving it to TestOptionsParser in > dart/tools/testing/dart/test_options.dart. > > Instead of the string 'The component to test against', print the contents of > this comment. I considered that, and it's a possibility, but the problem is that it doesn't fit in well with all the other flags that only require a single line of documentation. (Putting this big multiline string in the middle of all those other flags didn't look so good.) Perhaps I should change the existing comment to point people to this file?
OK, I followed Peter Ahe's suggestion to place the documention in test_options.dart next to the flag, so now you see the documentation printed when you do test.py --help
+1 lgtm https://chromiumcodereview.appspot.com/9599015/diff/6002/tools/testing/dart/t... File tools/testing/dart/test_options.dart (right): https://chromiumcodereview.appspot.com/9599015/diff/6002/tools/testing/dart/t... tools/testing/dart/test_options.dart:64: ''' Let's keep the old header too, it still helps to define what 'component' means.
https://chromiumcodereview.appspot.com/9599015/diff/6002/tools/testing/dart/t... File tools/testing/dart/test_options.dart (right): https://chromiumcodereview.appspot.com/9599015/diff/6002/tools/testing/dart/t... tools/testing/dart/test_options.dart:64: ''' On 2012/03/05 18:28:33, sigmund wrote: > Let's keep the old header too, it still helps to define OK, added summary description of this flag now. |
