Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11413104:
Modify apidoc to output JSON as well as HTML. (Closed)
|
Created:
8 years, 1 month ago by Jacob Modified:
8 years ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionThe JSON could be used in the future to dynamically rendered DartDocs in the browser.
Known issue: MDN docs for dart:html are not merged into the JSON.
The json_serializer code is ugly and should go away at some point soon. Please don't spend a lot of time picking it apart as it is largely throw away code.
Ignore the change to dart2js_mirror.dart
It is being reviewed separately at https://codereview.chromium.org/11418106/
BUG=
Committed: https://code.google.com/p/dart/source/detail?r=15358
Patch Set 1 : ready to review #
Total comments: 58
Patch Set 2 : Code review fixes #
Total comments: 31
Patch Set 3 : Code review fixes #
Messages
Total messages: 7 (0 generated)
Eep. Lots more comments than I initially intended to write. Feel free to ignore whatever doesn't make sense. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... File sdk/lib/_internal/dartdoc/lib/dartdoc.dart (right): https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/dartdoc.dart:752: libraries.add(new LibraryElement.fromMirror(library.qualifiedName, library)); Line too long. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... File sdk/lib/_internal/dartdoc/lib/json_serializer.dart (right): https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/json_serializer.dart:8: * with a more generic Dart serialization library when one is available. "Not intended for broad consumption" and "when one is available" sometimes turn into permanent solutions, and so I'd recommend one of the following: 1) Move this library into lib/src so that it's known to be an implementation detail and NOT something that's revealed as a library in the API. 2) Add doc comments to each of the methods in case this library is more useful/important than it appears now. It's easy to write docs now while you're here and harder to do later. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/json_serializer.dart:16: static String serialize(Object o) { All of these static methods should just be top level functions per the style guide. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/json_serializer.dart:23: if (o == null) { Can be shortened to "if (...) return;" https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/json_serializer.dart:48: mirror.getField(memberName).then((result) { Isn't it possible with these futures that some complete after this function is finished? I feel as though each of these serialize methods should return a future and then do the logic above once each of the futures completes. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/json_serializer.dart:58: if (members.indexOf(getterName) == -1) { Why not "if (!members.contains(getterName))" ? https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/json_serializer.dart:185: if (!_prettyPrint) { This could be: "return (!_prettyPrint) ? JSON.stringify(...) : _sb.toString();" https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... File sdk/lib/_internal/dartdoc/lib/universe_serializer.dart (right): https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:18: * We have to inject comments on the side as the Dart2Js mirror system doesn't I really wish Dart2Js mirrors captured comments. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:24: * Must be called before using. "... before using this library." https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:26: void initUniverseSerializer(CommentMap comments) { Is it possible to have a default value in case comments are not available perhaps? https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:40: /** children of the node. */ Uppercase "Children" in comment. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:52: } What does this return if the mirror is not a ClassMirror? Null? Should this be documented? https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:56: if (children == null) { Why not just initialize children to <Element>[] above? https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:67: LibraryElement.fromMirror(String name, LibraryMirror mirror): super('library') { Line too long. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:101: Reference baseclass; CamelCase "baseClass" maybe? https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:147: GetterElement.fromMirror(String name, MethodMirror mirror): super('property') { Line too long. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:151: if (mirror.isStatic) { For this and all the similar blocks below: "this.isBlah = mirror.isBlah;" https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:159: * [Element] describing a method which may be a regular method, a setter, or an operator. Line too long. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:167: MethodElement(String name, MethodMirror mirror, [String kind = 'method']): super(kind) { Line too long. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:167: MethodElement(String name, MethodMirror mirror, [String kind = 'method']): super(kind) { This could be "MethodElement(this.name, ...)" https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:167: MethodElement(String name, MethodMirror mirror, [String kind = 'method']): super(kind) { Also, isn't it possible to do: "MethodElement(...) : super(kind), isSetter = mirror.isSetter, isOperator = mirror.isOperator, ... { ... }" https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:202: ParameterElement.fromMirror(ParameterMirror mirror): super('param') { Same comment about initializers as above. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:220: VariableElement.fromMirror(String name, VariableMirror mirror): super('property') { Line too long. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:235: TypedefElement.fromMirror(String name, TypedefMirror mirror): super('typedef') { Line too long. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:251: Reference(var mirror) { Any way to make this API better? "Reference(Mirror mirror) {" ? https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:271: static String getId(Mirror mirror) { Whole method could be shortened, possibly to: "var prefix = (mirror is MemberMirror && memberMirror.owner != null) ? '${getId(memberMirror.owner)/' : ''; return '${prefix}${mirror.simpleName}';" https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:282: static String cleanName(String name) { "cleanName" is not descriptive enough, I think. Maybe "stripBlah" or else add a doc comment. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:283: if (name.indexOf('@') != -1) { Whole method could be: "return name.contains('@') ? name.substring(0, name.indexOf('@')) : name;"
replied to all comments. thanks for reviewing. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... File sdk/lib/_internal/dartdoc/lib/dartdoc.dart (right): https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/dartdoc.dart:752: libraries.add(new LibraryElement.fromMirror(library.qualifiedName, library)); On 2012/11/21 17:01:15, Andrei Mouravski wrote: > Line too long. Done. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... File sdk/lib/_internal/dartdoc/lib/json_serializer.dart (right): https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/json_serializer.dart:8: * with a more generic Dart serialization library when one is available. I believe there are permanent solutions already being worked on. I didn't write this code and don't want to massage it that much to try to make it production ready form given that it will be thrown away. Moved to lib/src On 2012/11/21 17:01:15, Andrei Mouravski wrote: > "Not intended for broad consumption" and "when one is available" sometimes turn > into permanent solutions, and so I'd recommend one of the following: > > 1) Move this library into lib/src so that it's known to be an implementation > detail and NOT something that's revealed as a library in the API. > > 2) Add doc comments to each of the methods in case this library is more > useful/important than it appears now. It's easy to write docs now while you're > here and harder to do later. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/json_serializer.dart:16: static String serialize(Object o) { On 2012/11/21 17:01:15, Andrei Mouravski wrote: > All of these static methods should just be top level functions per the style > guide. Done. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/json_serializer.dart:23: if (o == null) { On 2012/11/21 17:01:15, Andrei Mouravski wrote: > Can be shortened to "if (...) return;" Done. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/json_serializer.dart:48: mirror.getField(memberName).then((result) { On 2012/11/21 17:01:15, Andrei Mouravski wrote: > Isn't it possible with these futures that some complete after this function is > finished? > > I feel as though each of these serialize methods should return a future and then > do the logic above once each of the futures completes. Methods use futures but are async. this code would be a pain to write truly async. as i said. not worth polishing this. added a TODO warning about this. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/json_serializer.dart:58: if (members.indexOf(getterName) == -1) { On 2012/11/21 17:01:15, Andrei Mouravski wrote: > Why not "if (!members.contains(getterName))" ? Done. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/json_serializer.dart:58: if (members.indexOf(getterName) == -1) { On 2012/11/21 17:01:15, Andrei Mouravski wrote: > Why not "if (!members.contains(getterName))" ? Done. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/json_serializer.dart:185: if (!_prettyPrint) { On 2012/11/21 17:01:15, Andrei Mouravski wrote: > This could be: > "return (!_prettyPrint) ? JSON.stringify(...) : > _sb.toString();" Done. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/json_serializer.dart:185: if (!_prettyPrint) { On 2012/11/21 17:01:15, Andrei Mouravski wrote: > This could be: > "return (!_prettyPrint) ? JSON.stringify(...) : > _sb.toString();" I would but I want to comment that the way we get the non pretty printed version is quite the hack. removed the ! and swapped the order of the clauses for more clarity https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... File sdk/lib/_internal/dartdoc/lib/universe_serializer.dart (right): https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:18: * We have to inject comments on the side as the Dart2Js mirror system doesn't On 2012/11/21 17:01:15, Andrei Mouravski wrote: > I really wish Dart2Js mirrors captured comments. ack. although i think the current design is correct... i mean you wouldn't want a deployed app to function differently if comments were removed. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:18: * We have to inject comments on the side as the Dart2Js mirror system doesn't On 2012/11/21 17:01:15, Andrei Mouravski wrote: > I really wish Dart2Js mirrors captured comments. Yeah that would be nice. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:24: * Must be called before using. On 2012/11/21 17:01:15, Andrei Mouravski wrote: > "... before using this library." Done. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:24: * Must be called before using. On 2012/11/21 17:01:15, Andrei Mouravski wrote: > "... before using this library." Done. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:26: void initUniverseSerializer(CommentMap comments) { On 2012/11/21 17:01:15, Andrei Mouravski wrote: > Is it possible to have a default value in case comments are not available > perhaps? It would be possible but I think implementing it would be bad software design. If it is needed it can be added later at low cost. At the moment there is no need at all for that use case so it would just cause code bloat or possible user errors if users forgot to pass in a commentMap to the init method. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:40: /** children of the node. */ On 2012/11/21 17:01:15, Andrei Mouravski wrote: > Uppercase "Children" in comment. Done. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:52: } On 2012/11/21 17:01:15, Andrei Mouravski wrote: > What does this return if the mirror is not a ClassMirror? Null? Should this be > documented? Dead bogus code. Removed https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:56: if (children == null) { On 2012/11/21 17:01:15, Andrei Mouravski wrote: > Why not just initialize children to <Element>[] above? Here and elsewhere we keep fields as null unless they have a non-default value. This makes the serialized JSON output smaller as null fields are omitted. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:67: LibraryElement.fromMirror(String name, LibraryMirror mirror): super('library') { On 2012/11/21 17:01:15, Andrei Mouravski wrote: > Line too long. Done. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:101: Reference baseclass; On 2012/11/21 17:01:15, Andrei Mouravski wrote: > CamelCase "baseClass" maybe? I think baseclass can be viewed as one word not two. superclass is definitely one word and I prefer to have consistency. On the other hand, i have no idea why this was called baseclass not superclass. Pete can probably chime in on that. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:147: GetterElement.fromMirror(String name, MethodMirror mirror): super('property') { On 2012/11/21 17:01:15, Andrei Mouravski wrote: > Line too long. Done. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:151: if (mirror.isStatic) { On 2012/11/21 17:01:15, Andrei Mouravski wrote: > For this and all the similar blocks below: > "this.isBlah = mirror.isBlah;" There is method to the madness. The key is to keep the fields null if they have the default value (false) so the json is concise. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:167: MethodElement(String name, MethodMirror mirror, [String kind = 'method']): super(kind) { On 2012/11/21 17:01:15, Andrei Mouravski wrote: > Line too long. Done. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:167: MethodElement(String name, MethodMirror mirror, [String kind = 'method']): super(kind) { On 2012/11/21 17:01:15, Andrei Mouravski wrote: > This could be "MethodElement(this.name, ...)" Done. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:167: MethodElement(String name, MethodMirror mirror, [String kind = 'method']): super(kind) { not desired. see above On 2012/11/21 17:01:15, Andrei Mouravski wrote: > Also, isn't it possible to do: > "MethodElement(...) > : super(kind), > isSetter = mirror.isSetter, > isOperator = mirror.isOperator, > ... { > ... > }" https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:167: MethodElement(String name, MethodMirror mirror, [String kind = 'method']): super(kind) { On 2012/11/21 17:01:15, Andrei Mouravski wrote: > This could be "MethodElement(this.name, ...)" Sadly that doesn't work because name is defined in the parent class. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:202: ParameterElement.fromMirror(ParameterMirror mirror): super('param') { On 2012/11/21 17:01:15, Andrei Mouravski wrote: > Same comment about initializers as above. same as above https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:251: Reference(var mirror) { On 2012/11/21 17:01:15, Andrei Mouravski wrote: > Any way to make this API better? > "Reference(Mirror mirror) {" ? Done. https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:271: static String getId(Mirror mirror) { Shorter isn't always better. I'm not convinced that is more readable. On 2012/11/21 17:01:15, Andrei Mouravski wrote: > Whole method could be shortened, possibly to: > "var prefix = (mirror is MemberMirror && memberMirror.owner != null) ? > '${getId(memberMirror.owner)/' : ''; > return '${prefix}${mirror.simpleName}';" https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:282: static String cleanName(String name) { On 2012/11/21 17:01:15, Andrei Mouravski wrote: > "cleanName" is not descriptive enough, I think. Maybe "stripBlah" or else add a > doc comment. not a big fan of this method but i think it is reasonable to call this cleanName. If other cleaning needs to be performed then the method name would stay the same.. otherwise your method name becomes stripBlaAndBazAndFoo https://codereview.chromium.org/11413104/diff/4001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:283: if (name.indexOf('@') != -1) { On 2012/11/21 17:01:15, Andrei Mouravski wrote: > Whole method could be: > "return name.contains('@') ? name.substring(0, name.indexOf('@')) : name;" Done.
https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... File sdk/lib/_internal/dartdoc/lib/dartdoc.dart (right): https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/dartdoc.dart:149: var libraries = <LibraryElement>[]; Can you just use _sortedLibraries instead of storing an additional field? https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/dartdoc.dart:254: initUniverseSerializer(_comments); This feels error-prone to me. How about just passing _comments into new LibraryElement instead of having global state? https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/dartdoc.dart:366: write(json_serializer.serialize(libraries)); var libraries = _sortedLibraries.map((lib) => new LibraryElement.fromMirror(lib.qualifiedName, lib)); ? https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... File sdk/lib/_internal/dartdoc/lib/universe_serializer.dart (right): https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:24: * Must be called before using this library. Just FYI, you can also use line doc comments: /// Must be called... A bit cleaner, especially for short doc comments, IMO. https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:35: String kind; Can you make these fields final? https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:45: Element(String kind) { this.kind here and then get rid of the body. https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:61: LibraryElement.fromMirror(String name, LibraryMirror mirror) Since these types only have one constructor, why make it a named one? Seems a bit verbose. https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:62: : super('library') { Style nit, indent +2 more. Here and elsewhere. https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:63: this.name = name; Use an initializing formal, or put in the ctor initialization list? https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:65: comment = _comments.findLibrary(mirror.location); Instead of assigning this, how about making it a parameter to Element's ctor? https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:96: Reference baseclass; "baseclass" -> "superclass". https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:146: this.ref = new Reference(mirror.returnType); How about moving these into the initializer list and then making them final? (Elsewhere too.) https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:248: List<Reference> variables; Are these the type parameters (as opposed to arguments)? Like "T" in "List<T>"? If so, "variables" -> "parameters". It can be helpful to track them. If, for example, you click "T" in "mappedBy(callback(T))", it would be good if it took you to the surrounding class definition so you can see where T comes from and any constraints it has ("T extends Blah"). https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:282: return name.contains('@') ? name.substring(0, name.indexOf('@')) : name; Add a comment explaining why a name might have that?
lgtm 1 nit. https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... File sdk/lib/_internal/dartdoc/lib/universe_serializer.dart (right): https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:234: TypedefElement.fromMirror(String name, TypedefMirror mirror): super('typedef') { Line still too long.
https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... File sdk/lib/_internal/dartdoc/lib/dartdoc.dart (right): https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/dartdoc.dart:149: var libraries = <LibraryElement>[]; On 2012/11/26 19:34:22, Bob Nystrom wrote: > Can you just use _sortedLibraries instead of storing an additional field? Done. https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/dartdoc.dart:254: initUniverseSerializer(_comments); Wish we had some decent dependency injection support. Tweaked the code to pass this around. This should really just be a comments service that you inject in, etc. On 2012/11/26 19:34:22, Bob Nystrom wrote: > This feels error-prone to me. How about just passing _comments into new > LibraryElement instead of having global state? i'd rather not pass the comment map around everywhere. seems like a much more verbose way to write this code without any real value. the comment oracle should be a service available on the side rather than something you pass around. https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/dartdoc.dart:366: write(json_serializer.serialize(libraries)); On 2012/11/26 19:34:22, Bob Nystrom wrote: > var libraries = _sortedLibraries.map((lib) => new > LibraryElement.fromMirror(lib.qualifiedName, lib)); > > ? Done. https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... File sdk/lib/_internal/dartdoc/lib/universe_serializer.dart (right): https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:24: * Must be called before using this library. On 2012/11/26 19:34:22, Bob Nystrom wrote: > Just FYI, you can also use line doc comments: > > /// Must be called... > > A bit cleaner, especially for short doc comments, IMO. I dislike files with a mix of /// and /** comments Aside from that I like /// a lot better for short comments. I'd be a fan for doing a fixit of auto-converting all /** comments to /// the only manual step would be fixing lines that became 81 chars. https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:35: String kind; On 2012/11/26 19:34:22, Bob Nystrom wrote: > Can you make these fields final? Made those fields final and reworked all the constructors for subclasses. https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:45: Element(String kind) { On 2012/11/26 19:34:22, Bob Nystrom wrote: > this.kind here and then get rid of the body. Done. https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:61: LibraryElement.fromMirror(String name, LibraryMirror mirror) On 2012/11/26 19:34:22, Bob Nystrom wrote: > Since these types only have one constructor, why make it a named one? Seems a > bit verbose. agreed. removed. https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:62: : super('library') { On 2012/11/26 19:34:22, Bob Nystrom wrote: > Style nit, indent +2 more. Here and elsewhere. Done. https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:63: this.name = name; On 2012/11/26 19:34:22, Bob Nystrom wrote: > Use an initializing formal, or put in the ctor initialization list? added to ctor initialization list https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:65: comment = _comments.findLibrary(mirror.location); On 2012/11/26 19:34:22, Bob Nystrom wrote: > Instead of assigning this, how about making it a parameter to Element's ctor? Done. https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:65: comment = _comments.findLibrary(mirror.location); On 2012/11/26 19:34:22, Bob Nystrom wrote: > Instead of assigning this, how about making it a parameter to Element's ctor? Done. https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:96: Reference baseclass; On 2012/11/26 19:34:22, Bob Nystrom wrote: > "baseclass" -> "superclass". Done. https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:146: this.ref = new Reference(mirror.returnType); On 2012/11/26 19:34:22, Bob Nystrom wrote: > How about moving these into the initializer list and then making them final? > (Elsewhere too.) I've made everything that was moderately convenient to make final final. https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:234: TypedefElement.fromMirror(String name, TypedefMirror mirror): super('typedef') { On 2012/11/26 20:12:38, Andrei Mouravski wrote: > Line still too long. Done. https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:248: List<Reference> variables; On 2012/11/26 19:34:22, Bob Nystrom wrote: > Are these the type parameters (as opposed to arguments)? Like "T" in "List<T>"? > If so, "variables" -> "parameters". > > It can be helpful to track them. If, for example, you click "T" in > "mappedBy(callback(T))", it would be good if it took you to the surrounding > class definition so you can see where T comes from and any constraints it has > ("T extends Blah"). Removed variables as they don't seem to be doing the right thing at the moment. https://codereview.chromium.org/11413104/diff/8001/sdk/lib/_internal/dartdoc/... sdk/lib/_internal/dartdoc/lib/universe_serializer.dart:282: return name.contains('@') ? name.substring(0, name.indexOf('@')) : name; On 2012/11/26 19:34:22, Bob Nystrom wrote: > Add a comment explaining why a name might have that? Turns out this wasn't needed anymore due to Dart2Js changes I believe. Removed.
LGTM! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
