Skip to content

Commit

Permalink
Fix a number of "Effective Dart" issues. (dart-lang#1050)
Browse files Browse the repository at this point in the history
* Fix a number of "Effective Dart" issues.

Tijo notes a bunch of good stuff:

- Some examples used "=>" for void members even though a guideline
  tells you not to do that.

- A typedef for a generic should have been a generic typedef.

- The background color for neutral code snippets is too similar to the
  bad background color for colorblind people. I somewhat mitigated this
  by turning a couple of the neutral examples into explicit good and bad
  ones.

  It's worth tweaking the neutral background color to be more distinct
  from the red background for red-green colorblindness, but I didn't
  feel comfortable making visual changes to the site.

* Avoid linter warning.
  • Loading branch information
munificent authored Aug 3, 2018
1 parent ddae2c4 commit 018207c
Show file tree
Hide file tree
Showing 7 changed files with 68 additions and 36 deletions.
12 changes: 9 additions & 3 deletions examples/misc/lib/effective_dart/docs_bad.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ void miscDeclAnalyzedButNotTested() {
/// certain operations may or may not be possible. If there is no file at
/// [path] or it can't be accessed, this function throws either [IOError]
/// or [PermissionError], respectively. Otherwise, this deletes the file.
void delete(String path) => ellipsis();
void delete(String path) {
ellipsis();
}
// #enddocregion first-sentence
}

Expand All @@ -24,7 +26,9 @@ void miscDeclAnalyzedButNotTested() {
/// Deletes the file at [path]. Throws an [IOError] if the file could not
/// be found. Throws a [PermissionError] if the file is present but could
/// not be deleted.
void delete(String path) => ellipsis();
void delete(String path) {
ellipsis();
}
// #enddocregion first-sentence-a-paragraph
}
}
Expand All @@ -39,7 +43,9 @@ class Widget {}
class RadioButtonWidget extends Widget {
/// Sets the tooltip for this radio button widget to the list of strings in
/// [lines].
void tooltip(List<String> lines) => ellipsis();
void tooltip(List<String> lines) {
ellipsis();
}
}
// #enddocregion redundant

Expand Down
16 changes: 12 additions & 4 deletions examples/misc/lib/effective_dart/docs_good.dart
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ void miscDeclAnalyzedButNotTested() {
{
// #docregion first-sentence
/// Deletes the file at [path] from the file system.
void delete(String path) => ellipsis();
void delete(String path) {
ellipsis();
}
// #enddocregion first-sentence
}

Expand All @@ -40,7 +42,9 @@ void miscDeclAnalyzedButNotTested() {
///
/// Throws an [IOError] if the file could not be found. Throws a
/// [PermissionError] if the file is present but could not be deleted.
void delete(String path) => ellipsis();
void delete(String path) {
ellipsis();
}
// #enddocregion first-sentence-a-paragraph
}

Expand All @@ -50,7 +54,9 @@ void miscDeclAnalyzedButNotTested() {
bool all(bool predicate(T element)) => ellipsis();

/// Starts the stopwatch if not already running.
void start() => ellipsis();
void start() {
ellipsis();
}
// #enddocregion third-person
};

Expand Down Expand Up @@ -146,7 +152,9 @@ class Widget {}
class RadioButtonWidget extends Widget {
/// Sets the tooltip to [lines], which should have been word wrapped using
/// the current font.
void tooltip(List<String> lines) => ellipsis();
void tooltip(List<String> lines) {
ellipsis();
}
}
// #enddocregion redundant

Expand Down
2 changes: 1 addition & 1 deletion examples/misc/lib/effective_dart/style_good.dart
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ class SliderMenu {/* ... */}

class HttpRequest {/* ... */}

typedef Predicate = bool Function<T>(T value);
typedef Predicate<T> = bool Function(T value);
// #enddocregion type-names

//----------------------------------------------------------------------------
Expand Down
17 changes: 11 additions & 6 deletions examples/misc/test/effective_dart_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -25,20 +25,25 @@ void main() {
});

test('runtimeType', () {
_test() {
// #docregion list-from-2
expect(() {
// #docregion list-from-good
// Creates a List<int>:
var iterable = [1, 2, 3];

// Prints "List<int>":
print(iterable.toList().runtimeType);
// #enddocregion list-from-good
}, prints('List<int>\n'));

expect(() {
// #docregion list-from-bad
// Creates a List<int>:
var iterable = [1, 2, 3];

// Prints "List<dynamic>":
print(List.from(iterable).runtimeType);
// #enddocregion list-from-2
}

expect(_test, prints('List<int>\nList<dynamic>\n'));
// #enddocregion list-from-bad
}, prints('List<dynamic>\n'));
});

test('List.from<int>() from List<num>', () {
Expand Down
34 changes: 24 additions & 10 deletions src/_guides/language/effective-dart/documentation.md
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,9 @@ elsewhere for the solution to their problem.
<?code-excerpt "misc/lib/effective_dart/docs_good.dart (first-sentence)"?>
{% prettify dart %}
/// Deletes the file at [path] from the file system.
void delete(String path) => ...
void delete(String path) {
...
}
{% endprettify %}

{:.bad-style}
Expand All @@ -153,7 +155,9 @@ void delete(String path) => ...
/// certain operations may or may not be possible. If there is no file at
/// [path] or it can't be accessed, this function throws either [IOError]
/// or [PermissionError], respectively. Otherwise, this deletes the file.
void delete(String path) => ...
void delete(String path) {
...
}
{% endprettify %}

### DO separate the first sentence of a doc comment into its own paragraph.
Expand All @@ -173,7 +177,9 @@ like lists of classes and members.
///
/// Throws an [IOError] if the file could not be found. Throws a
/// [PermissionError] if the file is present but could not be deleted.
void delete(String path) => ...
void delete(String path) {
...
}
{% endprettify %}

{:.bad-style}
Expand All @@ -182,7 +188,9 @@ void delete(String path) => ...
/// Deletes the file at [path]. Throws an [IOError] if the file could not
/// be found. Throws a [PermissionError] if the file is present but could
/// not be deleted.
void delete(String path) => ...
void delete(String path) {
...
}
{% endprettify %}

### AVOID redundancy with the surrounding context.
Expand All @@ -199,7 +207,9 @@ spelled out in the doc comment. Instead, focus on explaining what the reader
class RadioButtonWidget extends Widget {
/// Sets the tooltip to [lines], which should have been word wrapped using
/// the current font.
void tooltip(List<String> lines) => ...
void tooltip(List<String> lines) {
...
}
}
{% endprettify %}

Expand All @@ -209,7 +219,9 @@ class RadioButtonWidget extends Widget {
class RadioButtonWidget extends Widget {
/// Sets the tooltip for this radio button widget to the list of strings in
/// [lines].
void tooltip(List<String> lines) => ...
void tooltip(List<String> lines) {
...
}
}
{% endprettify %}

Expand All @@ -225,7 +237,9 @@ The doc comment should focus on what the code *does*.
bool all(bool predicate(T element)) => ...

/// Starts the stopwatch if not already running.
void start() => ...
void start() {
...
}
{% endprettify %}

### PREFER starting variable, getter, or setter comments with noun phrases.
Expand Down Expand Up @@ -288,7 +302,7 @@ constructor.

{:.good-style}
<?code-excerpt "misc/lib/effective_dart/docs_good.dart (identifiers)"?>
{% prettify none %}
{% prettify dart %}
/// Throws a [StateError] if ...
/// similar to [anotherMethod()], but ...
{% endprettify %}
Expand All @@ -298,7 +312,7 @@ separated by a dot:

{:.good-style}
<?code-excerpt "misc/lib/effective_dart/docs_good.dart (member)"?>
{% prettify none %}
{% prettify dart %}
/// Similar to [Duration.inDays], but handles fractional days.
{% endprettify %}

Expand All @@ -307,7 +321,7 @@ constructor, put parentheses after the class name:

{:.good-style}
<?code-excerpt "misc/lib/effective_dart/docs_good.dart (ctor)"?>
{% prettify none %}
{% prettify dart %}
/// To create a point, call [Point()] or use [Point.polar()] to ...
{% endprettify %}

Expand Down
2 changes: 1 addition & 1 deletion src/_guides/language/effective-dart/style.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class SliderMenu { ... }

class HttpRequest { ... }

typedef Predicate = bool Function<T>(T value);
typedef Predicate<T> = bool Function(T value);
{% endprettify %}

This even includes classes intended to be used in metadata annotations.
Expand Down
21 changes: 10 additions & 11 deletions src/_guides/language/effective-dart/usage.md
Original file line number Diff line number Diff line change
Expand Up @@ -330,20 +330,29 @@ The obvious difference is that the first one is shorter. The *important*
difference is that the first one preserves the type argument of the original
object:

<?code-excerpt "misc/test/effective_dart_test.dart (list-from-2)"?>
{:.good-style}
<?code-excerpt "misc/test/effective_dart_test.dart (list-from-good)"?>
{% prettify dart %}
// Creates a List<int>:
var iterable = [1, 2, 3];

// Prints "List<int>":
print(iterable.toList().runtimeType);
{% endprettify %}

{:.bad-style}
<?code-excerpt "misc/test/effective_dart_test.dart (list-from-bad)"?>
{% prettify dart %}
// Creates a List<int>:
var iterable = [1, 2, 3];

// Prints "List<dynamic>":
print(List.from(iterable).runtimeType);
{% endprettify %}

If you *want* to change the type, then calling `List.from()` is useful:

{:.good-style}
<?code-excerpt "misc/test/effective_dart_test.dart (list-from-3)"?>
{% prettify dart %}
var numbers = [1, 2.3, 4]; // List<num>.
Expand All @@ -357,16 +366,6 @@ you don't care about the type, then use `toList()`.

### DO use `whereType()` to filter a collection by type.

{% comment %}
update-for-dart-2
{% endcomment %}
<aside class="alert alert-warning" markdown="1">
**Before using `whereType()`, make sure it's implemented.**
We expect `whereType()` to be added late in Dart&nbsp;2 development.
For details, see
[SDK issue #32463.](https://github.com/dart-lang/sdk/issues/32463#issuecomment-402975456)
</aside>

Let's say you have a list containing a mixture of objects, and you want to get
just the integers out of it. You could use `where()` like this:

Expand Down

0 comments on commit 018207c

Please sign in to comment.