|
|
Created:
5 years, 2 months ago by srawlins Modified:
5 years, 2 months ago CC:
reviews_dartlang.org Base URL:
https://github.com/dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionFormatting comments for mirrors (MirrorSystem, IsolateMirror, DeclarationMirror, and ObjectMirror)
BUG=https://github.com/dart-lang/sdk/issues/24505
R=lrn@google.com
Committed: https://github.com/dart-lang/sdk/commit/fb7d8714286e76c1929eacafc9ce14a9f97e76bf
Patch Set 1 #
Total comments: 40
Patch Set 2 : addressing comments #Patch Set 3 : Address comments #Messages
Total messages: 9 (1 generated)
sethladd@google.com changed reviewers: + lrn@google.com, sethladd@google.com
https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dart File sdk/lib/mirrors/mirrors.dart (right): https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:73: /// Returns an immutable map from URIs to mirrors for all libraries known Why change this doc-comment's format, but not the class and library comments above? (Why change it at all?)
Why do we change the comment format of existing libraries? LGTM with comments if that is what we want. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dart File sdk/lib/mirrors/mirrors.dart (right): https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:74: /// to this mirror system. This is better described as a property: /// All libraries known to the mirror system, indexed by their URI. /// /// Returns an unmodifiable map of the libraries with [LibraryMirror.uri] /// as keys. /// ... the rest ... https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:119: /// and [library] is [:null:], throws an [ArgumentError]. If [name] is a Cosider changing [:...:] to `...`. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:125: /// the const constructor of Symbol or symbol literals. maybe: Symbol -> [Symbol] https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:159: /// Returns a [TypeMirror] reflecting the type represented by [key]. Inconsistent wording with the methods above. Maybe start with: /// Reflects a type. /// https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:180: /// Returns a unique name used to refer to an isolate in debugging messages. Describe a getter as a property: /// A unique name used to refer to the isolate in debugging messages. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:184: /// isolate. Otherwise returns [:false:]. /// Whether this mirror reflects the currently running isolate. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:187: /// Returns a [LibraryMirror] on the root library for this isolate. /// The root library for the reflected isolate. /// Also describe what a root library is - I'm assuming it is the script file used to start the Dart program or passed to Isolate.spawnUri. Isolates started with Isolate.spawn probably inherit the root library, even if they might never touch it. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:192: /// Otherwise returns [:false:]. The equality holds if and only if /// Whether [other] is an [IsolateMirror] on the same isolate as this mirror. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:207: /// such as 'method' for a method, [:void method() {...}:] or 'mylibrary' for Confusing to use "method". Maybe change to "methodName", "myMethod" or just "foo". https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:233: /// A mirror on the owner of this Dart language entity. This is the Newlines before "This". And change "This" to "The owner" https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:234: /// declaration immediately surrounding the reflectee. Consider changing the final '.' to ':'. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:236: /// * For a library, the owner is [:null:]. These should probably be list items, so add "* " in front. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:239: /// * For a mixin application *S with M*, the owner is the owner of *M*. *S with M* -> `S with M` dittos below https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:249: /// Returns [:true:] if this declaration is considered private according to /// Whether this declaration is library private. /// /// Always returns `false` for a library declaration, /// otherwise returns `true` if the declaration's name starts with an /// underscore character (`_`), and `false` if it doesn't. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:256: /// Is this declaration top-level? /// Whether this declaration is top-level. /// /// A declaration is considered top-level if its [owner] is a /// [LibraryMirror]. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:291: /// declaration. Interesting ... it does not include "static" or "external"? (Not relevant for this CL, but I should check if this is correct). https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:339: /// the private members of the class of *o* (otherwise). Reflow reduces the readability/editability of the original. Please do not reflow unless necessary (or if it doesn't matter). https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:362: /// Let *o* be the object reflected by this mirror, let *f* be the simple name *f* -> `f` More below.
Thanks for the review! I reverted the comment style change. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dart File sdk/lib/mirrors/mirrors.dart (right): https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:73: /// Returns an immutable map from URIs to mirrors for all libraries known On 2015/10/07 08:32:25, Lasse Reichstein Nielsen wrote: > Why change this doc-comment's format, but not the class and library comments > above? > (Why change it at all?) Sorry, in some other libraries I've been changing comment style as I go. Reverted back to the old style. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:74: /// to this mirror system. On 2015/10/07 09:09:03, Lasse Reichstein Nielsen wrote: > This is better described as a property: > > /// All libraries known to the mirror system, indexed by their URI. > /// > /// Returns an unmodifiable map of the libraries with [LibraryMirror.uri] > /// as keys. > /// > ... the rest ... Done. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:119: /// and [library] is [:null:], throws an [ArgumentError]. If [name] is a On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > Cosider changing [:...:] to `...`. Done. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:125: /// the const constructor of Symbol or symbol literals. On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > maybe: Symbol -> [Symbol] Done. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:159: /// Returns a [TypeMirror] reflecting the type represented by [key]. On 2015/10/07 09:09:03, Lasse Reichstein Nielsen wrote: > Inconsistent wording with the methods above. > Maybe start with: > /// Reflects a type. > /// Done. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:180: /// Returns a unique name used to refer to an isolate in debugging messages. On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > Describe a getter as a property: > /// A unique name used to refer to the isolate in debugging messages. Done. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:184: /// isolate. Otherwise returns [:false:]. On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > /// Whether this mirror reflects the currently running isolate. Done. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:187: /// Returns a [LibraryMirror] on the root library for this isolate. On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > /// The root library for the reflected isolate. > /// > Also describe what a root library is - I'm assuming it is the script file used > to start the Dart program or passed to Isolate.spawnUri. > > Isolates started with Isolate.spawn probably inherit the root library, even if > they might never touch it. I'm probably not qualified to spell out the definition of a root library. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:192: /// Otherwise returns [:false:]. The equality holds if and only if On 2015/10/07 09:09:03, Lasse Reichstein Nielsen wrote: > /// Whether [other] is an [IsolateMirror] on the same isolate as this mirror. Done. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:207: /// such as 'method' for a method, [:void method() {...}:] or 'mylibrary' for On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > Confusing to use "method". Maybe change to "methodName", "myMethod" or just > "foo". Done. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:233: /// A mirror on the owner of this Dart language entity. This is the On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > Newlines before "This". And change "This" to "The owner" Done. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:234: /// declaration immediately surrounding the reflectee. On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > Consider changing the final '.' to ':'. Done. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:236: /// * For a library, the owner is [:null:]. On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > These should probably be list items, so add "* " in front. Done. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:239: /// * For a mixin application *S with M*, the owner is the owner of *M*. On 2015/10/07 09:09:03, Lasse Reichstein Nielsen wrote: > *S with M* -> `S with M` > dittos below Done. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:249: /// Returns [:true:] if this declaration is considered private according to On 2015/10/07 09:09:03, Lasse Reichstein Nielsen wrote: > /// Whether this declaration is library private. > /// > /// Always returns `false` for a library declaration, > /// otherwise returns `true` if the declaration's name starts with an > /// underscore character (`_`), and `false` if it doesn't. Done. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:256: /// Is this declaration top-level? On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > /// Whether this declaration is top-level. > /// > /// A declaration is considered top-level if its [owner] is a > /// [LibraryMirror]. > Done. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:291: /// declaration. On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > Interesting ... it does not include "static" or "external"? > (Not relevant for this CL, but I should check if this is correct). Acknowledged. https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:339: /// the private members of the class of *o* (otherwise). On 2015/10/07 09:09:03, Lasse Reichstein Nielsen wrote: > Reflow reduces the readability/editability of the original. > Please do not reflow unless necessary (or if it doesn't matter). This means it is equally not readable online: https://api.dartlang.org/133436/dart-mirrors/ObjectMirror/invoke.html There is currently not a way to hard newline in package:markdown paragraphs (https://github.com/dart-lang/markdown/issues/60). Should these be in a bulleted list? Or separate paragraphs? https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:362: /// Let *o* be the object reflected by this mirror, let *f* be the simple name On 2015/10/07 09:09:03, Lasse Reichstein Nielsen wrote: > *f* -> `f` > More below. This *f* format is everywhere in this file. Maybe that should be a global (to the file) change in a different CL.
On 2015/10/07 10:26:30, srawlins wrote: > Thanks for the review! > > I reverted the comment style change. > > https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dart > File sdk/lib/mirrors/mirrors.dart (right): > > https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... > sdk/lib/mirrors/mirrors.dart:73: /// Returns an immutable map from URIs to > mirrors for all libraries known > On 2015/10/07 08:32:25, Lasse Reichstein Nielsen wrote: > > Why change this doc-comment's format, but not the class and library comments > > above? > > (Why change it at all?) > > Sorry, in some other libraries I've been changing comment style as I go. > Reverted back to the old style. > > https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... > sdk/lib/mirrors/mirrors.dart:74: /// to this mirror system. > On 2015/10/07 09:09:03, Lasse Reichstein Nielsen wrote: > > This is better described as a property: > > > > /// All libraries known to the mirror system, indexed by their URI. > > /// > > /// Returns an unmodifiable map of the libraries with [LibraryMirror.uri] > > /// as keys. > > /// > > ... the rest ... > > Done. > > https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... > sdk/lib/mirrors/mirrors.dart:119: /// and [library] is [:null:], throws an > [ArgumentError]. If [name] is a > On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > > Cosider changing [:...:] to `...`. > > Done. > > https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... > sdk/lib/mirrors/mirrors.dart:125: /// the const constructor of Symbol or symbol > literals. > On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > > maybe: Symbol -> [Symbol] > > Done. > > https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... > sdk/lib/mirrors/mirrors.dart:159: /// Returns a [TypeMirror] reflecting the type > represented by [key]. > On 2015/10/07 09:09:03, Lasse Reichstein Nielsen wrote: > > Inconsistent wording with the methods above. > > Maybe start with: > > /// Reflects a type. > > /// > > Done. > > https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... > sdk/lib/mirrors/mirrors.dart:180: /// Returns a unique name used to refer to an > isolate in debugging messages. > On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > > Describe a getter as a property: > > /// A unique name used to refer to the isolate in debugging messages. > > Done. > > https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... > sdk/lib/mirrors/mirrors.dart:184: /// isolate. Otherwise returns [:false:]. > On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > > /// Whether this mirror reflects the currently running isolate. > > Done. > > https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... > sdk/lib/mirrors/mirrors.dart:187: /// Returns a [LibraryMirror] on the root > library for this isolate. > On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > > /// The root library for the reflected isolate. > > /// > > Also describe what a root library is - I'm assuming it is the script file used > > to start the Dart program or passed to Isolate.spawnUri. > > > > Isolates started with Isolate.spawn probably inherit the root library, even if > > they might never touch it. > > I'm probably not qualified to spell out the definition of a root library. > > https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... > sdk/lib/mirrors/mirrors.dart:192: /// Otherwise returns [:false:]. The equality > holds if and only if > On 2015/10/07 09:09:03, Lasse Reichstein Nielsen wrote: > > /// Whether [other] is an [IsolateMirror] on the same isolate as this mirror. > > Done. > > https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... > sdk/lib/mirrors/mirrors.dart:207: /// such as 'method' for a method, [:void > method() {...}:] or 'mylibrary' for > On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > > Confusing to use "method". Maybe change to "methodName", "myMethod" or just > > "foo". > > Done. > > https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... > sdk/lib/mirrors/mirrors.dart:233: /// A mirror on the owner of this Dart > language entity. This is the > On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > > Newlines before "This". And change "This" to "The owner" > > Done. > > https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... > sdk/lib/mirrors/mirrors.dart:234: /// declaration immediately surrounding the > reflectee. > On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > > Consider changing the final '.' to ':'. > > Done. > > https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... > sdk/lib/mirrors/mirrors.dart:236: /// * For a library, the owner is [:null:]. > On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > > These should probably be list items, so add "* " in front. > > Done. > > https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... > sdk/lib/mirrors/mirrors.dart:239: /// * For a mixin application *S with M*, the > owner is the owner of *M*. > On 2015/10/07 09:09:03, Lasse Reichstein Nielsen wrote: > > *S with M* -> `S with M` > > dittos below > > Done. > > https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... > sdk/lib/mirrors/mirrors.dart:249: /// Returns [:true:] if this declaration is > considered private according to > On 2015/10/07 09:09:03, Lasse Reichstein Nielsen wrote: > > /// Whether this declaration is library private. > > /// > > /// Always returns `false` for a library declaration, > > /// otherwise returns `true` if the declaration's name starts with an > > /// underscore character (`_`), and `false` if it doesn't. > > Done. > > https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... > sdk/lib/mirrors/mirrors.dart:256: /// Is this declaration top-level? > On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > > /// Whether this declaration is top-level. > > /// > > /// A declaration is considered top-level if its [owner] is a > > /// [LibraryMirror]. > > > > Done. > > https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... > sdk/lib/mirrors/mirrors.dart:291: /// declaration. > On 2015/10/07 09:09:02, Lasse Reichstein Nielsen wrote: > > Interesting ... it does not include "static" or "external"? > > (Not relevant for this CL, but I should check if this is correct). > > Acknowledged. > > https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... > sdk/lib/mirrors/mirrors.dart:339: /// the private members of the class of *o* > (otherwise). > On 2015/10/07 09:09:03, Lasse Reichstein Nielsen wrote: > > Reflow reduces the readability/editability of the original. > > Please do not reflow unless necessary (or if it doesn't matter). > > This means it is equally not readable online: > > https://api.dartlang.org/133436/dart-mirrors/ObjectMirror/invoke.html > > There is currently not a way to hard newline in package:markdown paragraphs > (https://github.com/dart-lang/markdown/issues/60). Should these be in a bulleted > list? Or separate paragraphs? > > https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... > sdk/lib/mirrors/mirrors.dart:362: /// Let *o* be the object reflected by this > mirror, let *f* be the simple name > On 2015/10/07 09:09:03, Lasse Reichstein Nielsen wrote: > > *f* -> `f` > > More below. > > This *f* format is everywhere in this file. Maybe that should be a global (to > the file) change in a different CL. I don't have commit access. Could you land?
https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dart File sdk/lib/mirrors/mirrors.dart (right): https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:339: /// the private members of the class of *o* (otherwise). I think bulleted would be better, maybe reworded as: This method performs the method invocation o.f(a1, ..., an, k1: v1, ..., km: vm) where * `o` is the object reflected by this mirror, * `f` is the simple name of the member denoted by [memberName], ... * and in a scope that has access to the private ... If the invocation returns a result ... If the invocation causes a compile-time error then ... If the invocation throws the object `e`, this method throws `e`.
https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dart File sdk/lib/mirrors/mirrors.dart (right): https://codereview.chromium.org/1383403002/diff/1/sdk/lib/mirrors/mirrors.dar... sdk/lib/mirrors/mirrors.dart:339: /// the private members of the class of *o* (otherwise). On 2015/10/07 10:26:29, srawlins wrote: > This means it is equally not readable online: I prefer to make it readable in one case rather than zero, although two is definitely better.
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as fb7d8714286e76c1929eacafc9ce14a9f97e76bf (presubmit successful). |