Fields, getters and setters documented as properties.

Fields, getters and setters documented as properties.

Committed: https://code.google.com/p/dart/source/detail?r=13611

[Bob Nystrom, 2012-10-11 17:04:21]

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart
File pkg/dartdoc/lib/dartdoc.dart (right):

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1012: final staticGetters = new
Map<String,MemberMirror>();
Style nit: there should be spaces after the commas here.

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1024: if (member.isGetter) {
What happened to the indentation here?

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1094: if (getters.length + setters.length > 0) {
Do an early return here and lose a level of indentation:

if (getters.isEmpty() && setters.isEmpty()) return;

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1106: MemberMirror setter = setters[name];
You can just use var here and elsewhere. Type annotations on locals make Dart
look painfully verbose.

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1161: * Documents the [member] in declared in
[host]. Handles all kinds of methods
"in declared" -> "declared"
"methods" -> "members"

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1237: write(' _)');
"_" -> "value"?

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1279: * setters.
This comment is out of sync with the method.

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1283: assert(getter != null);
We generally assume all parameters are non-null and don't bother asserting or
validating that. Why check getter here?

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1394: */
This comment is out of sync with the method.

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/src/client/cl...
File pkg/dartdoc/lib/src/client/client-shared.dart (right):

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/src/client/cl...
pkg/dartdoc/lib/src/client/client-shared.dart:41: var preList =
elem.queryAll('pre.source');
Nit, I would prefer "pres" or "sources" over "preList". For collection objects,
I prefer just using a plural name over mentioning the concrete collection type,
unless that really matters.

[Lasse Reichstein Nielsen, 2012-10-12 09:47:26]

lgtm

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart
File pkg/dartdoc/lib/dartdoc.dart (right):

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1012: final staticGetters = new
Map<String,MemberMirror>();
I'm getting less and less attached to that space (and I used to complain about
it missing too). 
It really breaks up the type and makes it harder to scan pat them to get from
the "new ClassName" to the parameters.
I.e., I see, and read, "new Foo<Bar,Baz>()" by seeing that it's a
"new"-expression, it's to the type Foo with some type parameters, and then I
skip to read the parameters. Only if I have reason to check will I actually read
the type parameters. Scanning "new Foo<Bar, Baz>()" is harder because I stop at
the space and need to mentally recognize that I need to continue.

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1073: operatorOrder.indexOf(b.simpleName));
How inefficient. How about making the operatorOrder a Map<String,int> that
returns the priority directly, instead of doing a linear lookup in a list twice
per comparison in the sort?

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1094: if (getters.length + setters.length > 0) {
Agree violently.

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1099: return
a.toLowerCase().compareTo(b.toLowerCase());
Seems wasteful to do toLowerCase twice on every call (but I know you've asked
for compareToIgnoreCase :).

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1106: MemberMirror setter = setters[name];
I disagree. I can't read the type of 'getter' from the right-hand-side, and
really appreciate (and demand!) the type be there to inform me.
Verbose, yes, but the pain to me is when it's not there, and I have to think
every time I read the line.

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1237: write(' _)');
Let's do that. C# programmers will be happy.

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/src/client/cl...
File pkg/dartdoc/lib/src/client/client-shared.dart (right):

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/src/client/cl...
pkg/dartdoc/lib/src/client/client-shared.dart:41: var preList =
elem.queryAll('pre.source');
Agree, and the HTML tag seems less interesting than the class in describing the
elements, so "sources".

[Johnni Winther, 2012-10-12 11:15:53]

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart
File pkg/dartdoc/lib/dartdoc.dart (right):

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1012: final staticGetters = new
Map<String,MemberMirror>();
On 2012/10/12 09:47:26, Lasse Reichstein Nielsen wrote:
> I'm getting less and less attached to that space (and I used to complain about
> it missing too). 
> It really breaks up the type and makes it harder to scan pat them to get from
> the "new ClassName" to the parameters.
> I.e., I see, and read, "new Foo<Bar,Baz>()" by seeing that it's a
> "new"-expression, it's to the type Foo with some type parameters, and then I
> skip to read the parameters. Only if I have reason to check will I actually
read
> the type parameters. Scanning "new Foo<Bar, Baz>()" is harder because I stop
at
> the space and need to mentally recognize that I need to continue.

I would even vote for a style guide change. Consider for example the signature
in lines 1091-1093. Putting spaces in the type annotation would make it look
there are more parameters than is the case.

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1024: if (member.isGetter) {
On 2012/10/11 17:04:21, Bob Nystrom wrote:
> What happened to the indentation here?

Done.

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1073: operatorOrder.indexOf(b.simpleName));
On 2012/10/12 09:47:26, Lasse Reichstein Nielsen wrote:
> How inefficient. How about making the operatorOrder a Map<String,int> that
> returns the priority directly, instead of doing a linear lookup in a list
twice
> per comparison in the sort?

Done.

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1094: if (getters.length + setters.length > 0) {
On 2012/10/11 17:04:21, Bob Nystrom wrote:
> Do an early return here and lose a level of indentation:
> 
> if (getters.isEmpty() && setters.isEmpty()) return;

Done.

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1161: * Documents the [member] in declared in
[host]. Handles all kinds of methods
On 2012/10/11 17:04:21, Bob Nystrom wrote:
> "in declared" -> "declared"
> "methods" -> "members"

Done.

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1237: write(' _)');
On 2012/10/11 17:04:21, Bob Nystrom wrote:
> "_" -> "value"?

Done.

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1279: * setters.
On 2012/10/11 17:04:21, Bob Nystrom wrote:
> This comment is out of sync with the method.

Done.

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1283: assert(getter != null);
On 2012/10/11 17:04:21, Bob Nystrom wrote:
> We generally assume all parameters are non-null and don't bother asserting or
> validating that. Why check getter here?

[setter] might be null, but [getter] may not.

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1394: */
On 2012/10/11 17:04:21, Bob Nystrom wrote:
> This comment is out of sync with the method.

Done.

[Bob Nystrom, 2012-10-12 16:09:02]

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart
File pkg/dartdoc/lib/dartdoc.dart (right):

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1012: final staticGetters = new
Map<String,MemberMirror>();
On 2012/10/12 11:15:53, Johnni Winther wrote:
> On 2012/10/12 09:47:26, Lasse Reichstein Nielsen wrote:
> > I'm getting less and less attached to that space (and I used to complain
about
> > it missing too). 
> > It really breaks up the type and makes it harder to scan pat them to get
from
> > the "new ClassName" to the parameters.
> > I.e., I see, and read, "new Foo<Bar,Baz>()" by seeing that it's a
> > "new"-expression, it's to the type Foo with some type parameters, and then I
> > skip to read the parameters. Only if I have reason to check will I actually
> read
> > the type parameters. Scanning "new Foo<Bar, Baz>()" is harder because I stop
> at
> > the space and need to mentally recognize that I need to continue.
> 
> I would even vote for a style guide change. Consider for example the signature
> in lines 1091-1093. Putting spaces in the type annotation would make it look
> there are more parameters than is the case.

I personally don't have any trouble reading it with spaces after the ",". Java's
style of not using "," only after type parameters always seemed arbitrary to me
but I don't have a strong preference one way or the other.

At least for now, the style guideline is that there should be a space after it.
If you'd like, we discuss revising it to but that should be a dart-team@
discussion and not just here.

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1106: MemberMirror setter = setters[name];
On 2012/10/12 09:47:26, Lasse Reichstein Nielsen wrote:
> I disagree. I can't read the type of 'getter' from the right-hand-side, and
> really appreciate (and demand!) the type be there to inform me.

That's the tool's job! :) Programmers shouldn't have to manually add redundant
information to their code that can be automated.

One thing I'm very sensitive to is the aesthetics of Dart code. People will
judge the language by how a page of code in it looks. To anyone coming from
JavaScript, Python, C#, or pretty much any language except Java, type
annotations on locals are a very strong turn-off.

I don't think the (negligible) additional information that local type annotation
provides is worth alienating potential users.

> Verbose, yes, but the pain to me is when it's not there, and I have to think
> every time I read the line.

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1237: write(' _)');
On 2012/10/12 11:15:53, Johnni Winther wrote:
> On 2012/10/11 17:04:21, Bob Nystrom wrote:
> > "_" -> "value"?
> 
> Done.

Yay!

[Lasse Reichstein Nielsen, 2012-10-15 09:16:20]

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart
File pkg/dartdoc/lib/dartdoc.dart (right):

https://codereview.chromium.org/11028138/diff/1/pkg/dartdoc/lib/dartdoc.dart#...
pkg/dartdoc/lib/dartdoc.dart:1106: MemberMirror setter = setters[name];
On 2012/10/12 16:09:02, Bob Nystrom wrote:
> On 2012/10/12 09:47:26, Lasse Reichstein Nielsen wrote:
> > I disagree. I can't read the type of 'getter' from the right-hand-side, and
> > really appreciate (and demand!) the type be there to inform me.
> 
> That's the tool's job! :) 

I'm very much in the "must be readable when printed" faction here. You can't
control which tools are used to view or edit the code, so it should be readable
in all of them.

> Programmers shouldn't have to manually add redundant
> information to their code that can be automated.

By all means use a pre-processor to add the types before committing :)
 
> One thing I'm very sensitive to is the aesthetics of Dart code. People will
> judge the language by how a page of code in it looks. To anyone coming from
> JavaScript, Python, C#, or pretty much any language except Java, type
> annotations on locals are a very strong turn-off.

That might be my problem - I come from both (enterprise) Java and JavaScript,
and I find Java easier to read. Not easier to write, but that wasn't the design
intent of Java - it is designed to always be readable by making nothing
implicit. 
 
> I don't think the (negligible) additional information that local type
annotation
> provides is worth alienating potential users.

It's not negligible to me - I can't read and understand the code without that
information. If I had written the code originally, or edited it, or if I had had
to read the entire file (or library) first, I would probably understand it, but
as an outsider, the code is simply not understandable without more information. 
You can put the extra information in the variable name (setterMirror), or you
can add it as a type assertion (MemberMirror setter, giving you a shorter name
elsewhere), I won't care, but omitting it isn't good for readability, and
therefore not good for maintainability.
If the code isn't just a quick hack (which it is great that we also support),
maintainability is more important than ... pretty much everything except
correctness.

> > Verbose, yes, but the pain to me is when it's not there, and I have to think
> > every time I read the line.
>