Revise the guidelines around types. #889
Conversation
After a lot of discussion with readability and language team folks, I have a better understanding of what the consensus style is and how to express it in guidelines.
googlebot
added
the
cla: yes
| var names = people.map((Person person) { | ||
| return person.name; | ||
| }); | ||
| var names = people.map((Person person) => person.name); |
|
|
||
|
|
||
| ### DO type annotate public declarations whose type isn't inferred. | ||
| ### PREFER type annotating public top-level variables and fields if the type isn't obvious. |
There was a problem hiding this comment.
How about: top-level variables and fields --> fields and top-level variables
| { | ||
| // #docregion redundant-explicit | ||
| var things = new Set(); | ||
| // #enddocregion redundant-explicit |
There was a problem hiding this comment.
nit: maybe name this region simply explicit (both here and in the design_good.dart file), because there is no redundancy if one must be explicit :) (Yes, I know that it is probably because this example comes under the "redundant" guideline.)
| return highest; | ||
| } | ||
| {% endprettify %} | ||
| * What types should I write because I think it's best for them to be visible in |
| These guidelines help you answer the first question: | ||
|
|
||
| * [PREFER type annotating public top-level variables and fields if the type isn't obvious.](#prefer-type-annotating-public-top-level-variables-and-fields-if-the-type-isnt-obvious) | ||
| * [CONSIDER type annotating private top-level variables and fields if the type isn't obvious.](#consider-type-annotating-private-top-level-variables-and-fields-if-the-type-isnt-obvious) |
There was a problem hiding this comment.
For both of these guidelines I think that we should write "fields" before "top-level variables". E.g.,
PREFER type annotating public fields and top-level variables if ...
| @@ -1183,7 +1168,7 @@ List<List<Ingredient>> possibleDesserts(Set<Ingredient> pantry) { | |||
| {% endprettify %} | |||
|
|
|||
| If the local variable doesn't have an initializer, then its type can't be | |||
| inferred. In that case, it *is* a good idea to annotate it. Otherwise, you get | |||
| inferred. In that case, it *is* a good idea to annotate. Otherwise, you get | |||
| `dynamic` and lose the benefits of types. | |||
There was a problem hiding this comment.
Consider:
lose the benefits of types --> lose the benefits of static type checking
| {% endprettify %} | ||
|
|
||
| Here, the type annotation on the variable is used to infer the type argument of | ||
| constructor call in the initializer. In cases like this where inference produces |
| Here, the type annotation on the variable is used to infer the type argument of | ||
| constructor call in the initializer. In cases like this where inference produces | ||
| the right type, it's usually abundantly obvious what the type argument will be | ||
| and writing it again is just noise. |
There was a problem hiding this comment.
Come to think of it, for sake of brievity, I'd consider dropping the sentence "In cases like this ... abundantly obvious ... just noise."
There was a problem hiding this comment.
Fair. Done. I thought it might help to provide some motivation, but it's probably overkill.
| declarations, this guideline just restates that you should prefer typing "on the | ||
| left". | ||
| Here, since the variable has no type annotation, there isn't enough context to | ||
| determine what kind of Set to create, so the type argument should be provided |
| put a type annotation in the initializer to get inference working, then typing | ||
| the local variable itself is the way to go. | ||
| {:.bad-style} | ||
| <?code-excerpt "misc/lib/effective_dart/design_bad.dart (inferred-wrong)" replace="/ +\/\/ ignore: invalid_assignment\n//g"?> |
There was a problem hiding this comment.
You can probably shorten the replace expression:
replace="/ +\/\/ ignore: .*?\n//g"
|
Thanks! |
| return highest; | ||
| } | ||
| {% endprettify %} | ||
| * Which types should I write because inference can't provide them for me? | ||
|
|
There was a problem hiding this comment.
Delete the blank line between these items, to make them more obviously a group. (Plus, they're shorter than the no-extra-space-between-items list that follows.)
| Type annotations are important documentation for how a library should be used. | ||
| They form boundaries between regions of a program to isolate the source of a | ||
| type error. | ||
| Consider: |
There was a problem hiding this comment.
This should be a sentence explaining what to consider.
Also... this doesn't look like a variable or field to me. What does this section really cover?
There was a problem hiding this comment.
Oops. Moved some stuff around and left this hanging. Fixed.
| as important is guiding *maintainers* of your code. Adding type annotations to | ||
| private member and variable declarations can help future readers of your code | ||
| understand it, and help corral bugs. | ||
| ### CONSIDER type annotating private top-level variables and fields if the type isn't obvious. |
There was a problem hiding this comment.
Adjust to match whatever you do to the title of the previous guideline.
| that energy choosing a great *name* for your local variable. | ||
| Local variables, especially in modern code where functions tend to be small, | ||
| have very little scope. Omitting the type focuses the reader's attention on the | ||
| more important *name* of the variable and its initialized value. |
There was a problem hiding this comment.
Why do the two examples have different local variable names? I found myself distracted by that, because I expected only the type annotations to be different.
There was a problem hiding this comment.
The idea, which probably is lost on the reader, was that by not worrying about the type annotation, the author had the time to choose a better name. Probably trying to be too clever. Changed to use the same names.
After a lot of discussion with readability and language team folks, I
have a better understanding of what the consensus style is and how to
express it in guidelines.