Library tour example tests, up to but excluding dart:html #440

chalin · 2017-11-20T20:09:46Z

Contributes to #407 and #416.

Contributes to dart-lang#407 and dart-lang#416.

chalin · 2017-11-20T21:10:14Z

I forgot to mention that like for the Language Tour, the main file to review in this PR is src/_guides/libraries/library-tour.md. Most of the changes to that file are updated <?code-excerpt?> paths, but a few of the code excerpts have been updated too.

chalin · 2017-11-20T21:11:37Z

src/_guides/libraries/library-tour.md

@@ -984,6 +974,49 @@ Future objects appear throughout the Dart libraries, often as the object
 returned by an asynchronous method. When a future *completes*, its value
 is ready to use.

+
+#### Basic usage


This subsection isn't new, I just moved it up so that basic usage (explaining Future.then() etc) comes just before "Using await".

FWIW, I'm not sure this is a good move. Most developers, most of the time, will want to use async/await. Maybe we want to call this section "low-level usage" and move it back?

That's what I figured, but the presentation seemed off on first reading. I.e., the "Using await" section, when first, uses an example of "Basic usage" to motivate the use of await.

Anyhow, I'll revert to the original order.

chalin · 2017-11-20T21:12:47Z

src/_guides/libraries/library-tour.md

+  /* Handle exception... */
+}
+{% endprettify %}
+


Given that the subsection before this one already covers "Using await", it seemed to make sense to include a version of the example code that uses await rather than then() chaining

Just bookmarking in case you do decide to go with my suggestion to move "Basic usage" back to where it was. Not sure if it changes anything about the reasoning here. (I think it doesn't.)

Not sure if it changes anything about the reasoning here. (I think it doesn't.)

Agreed. Leaving this as is.

filiph

LGTM with minor comments

filiph · 2017-11-20T23:02:40Z

examples/lib/library_tour/async/future.dart

+import 'dart:async';
+// #enddocregion import
+
+void miscDeclAnalyzedButNotTested() {


Is there a rule/heuristic for when code snippets are tested and when they aren't? If so, it would be good to document it somewhere.

Ok, I'll add a few lines to the README when it is revised to have adopt writer-oriented perspective. How's this:

Most code excerpts should be tested, unless they are so incomplete that they require extensive supplemental support infrastructure (which isn't being shown in the tour), just to get something executable.

IMHO, it makes more sense to invest in testing complete samples, when they exist, than to have to create considerable support code. Also, once the code samples are setup for processing of docregions, then we could replace some of the tour code excerpts with snippets from complete samples.

filiph · 2017-11-21T00:41:38Z

scripts/analyze-and-test-examples.sh

@@ -34,7 +34,9 @@ echo
 travis_fold start analyzeAndTest.tests.vm
 echo Running VM tests ...

-pub run test --reporter expanded --exclude-tags=browser | tee $LOG_FILE
+TEST="pub run test" # --reporter expanded


What are we disabling here with commenting out --reporter expanded, exactly? Might want to just delete it or make a separate comment. This way it looks like a malformed explanation comment.

Ah, I had meant to clean that out, thanks. (Running pub run test from a script makes --reporter expanded the default.)

Done.

filiph · 2017-11-21T00:42:59Z

src/_guides/libraries/library-tour.md

-[int,]({{site.dart_api}}/{{site.data.pkg-vers.SDK.channel}}/dart-core/int-class.html)
-[double,]({{site.dart_api}}/{{site.data.pkg-vers.SDK.channel}}/dart-core/double-class.html) and
-[num.]({{site.dart_api}}/{{site.data.pkg-vers.SDK.channel}}/dart-core/num-class.html) Also see
+[int,][int] [double,][double] and [num.][num] Also see


nit: [num][num]. I think the period and the commas should be outside the link.

Also, I believe you can just say [num][] in this case.

Right, but those render in the HTML as external links, which have an arrow icon afterwards:

Kathy much prefers having the punctuation before the "external" icon.

filiph · 2017-11-21T00:47:04Z

src/_guides/libraries/library-tour.md

@@ -536,18 +525,20 @@ Although Map doesn’t implement Iterable, you can get Iterables from it
 using the Map `keys` and `values` properties.
 </div>

-Use `isEmpty` to check whether a list, set, or map has no items:
+Use `isEmpty` or `isNotEmpty` to check whether a list, set, or map has items:


I wonder if we should add a comment explaining why the core library even has isNotEmpty. Something like "Practice showed that developers use !list.isEmpty a lot and so — after careful consideration — we decided to add isNotEmpty. It's redundant, but developers find it handy, readable and pragmatic."

cc @kwalrath

Consider this non-blocking. I'd love to get Kathy's opinion once she gets back.

SGTM, I'll add this to a list of topic to review with Kathy when she returns.

I don't want to add a comment this to the tour, since it generally tries to be as succinct as possible. If there's a place that talks about designing APIs (e.g. Effective Dart), that might be an appropriate place to mention this.

There is some form of justification given in this Effective Dart Usage Dont': https://www.dartlang.org/guides/language/effective-dart/usage#dont-use-length-to-see-if-a-collection-is-empty

Shall I make "Use isEmpty or isNotEmpty" a link to this entry?

filiph · 2017-11-21T00:51:26Z

src/_guides/libraries/library-tour.md

@@ -984,6 +974,49 @@ Future objects appear throughout the Dart libraries, often as the object
 returned by an asynchronous method. When a future *completes*, its value
 is ready to use.

+
+#### Basic usage


FWIW, I'm not sure this is a good move. Most developers, most of the time, will want to use async/await. Maybe we want to call this section "low-level usage" and move it back?

filiph · 2017-11-21T00:53:24Z

src/_guides/libraries/library-tour.md

+  /* Handle exception... */
+}
+{% endprettify %}
+


Just bookmarking in case you do decide to go with my suggestion to move "Basic usage" back to where it was. Not sure if it changes anything about the reasoning here. (I think it doesn't.)

filiph · 2017-11-21T00:56:14Z

src/_guides/libraries/library-tour.md

+await Future.wait([
+  deleteLotsOfFiles(),
+  copyLotsOfFiles(),
+  checksumLotsOfOtherFiles()


Nice.

Style-wise, this looks like a good place for a trailing comma (dartfmt will be forced to always put each element on its own line, and moving elements around is much easier).

Yes, good point. Done.

filiph · 2017-11-21T00:58:12Z

src/_guides/libraries/library-tour.md

 {% prettify dart %}
-main(List<String> arguments) async {
-  ...
+Future main(List<String> arguments) async {


I wonder if we can elide the return type Future in this section, just to keep the code at a minimum.

While we elided return type annotations for main() functions at the start of the Language Tour to keep the code as light as possible, IMHO, our Library Tour should illustrate the recommended practice of specifying the return type of public APIs.

Generally speaking, I find it easy to gloss over the "async" keyword, whereas the "Future" annotation at the start of the method signature is usually grabs one attention :).

I'd vote to keep it. If you'd still like for me to remove it, let me know.

I'd prefer to remove the type before main(). It doesn't seem to help, and it obscures the method name.

filiph · 2017-11-21T00:59:37Z

src/_guides/libraries/library-tour.md

@@ -2695,3 +2668,63 @@ sampling of what you can install using pub.

 To learn more about the Dart language, see
 [A Tour of the Dart Language](/guides/language/language-tour).
+
+[AnchorElement]: {{site.dart_api}}/{{site.data.pkg-vers.SDK.channel}}/dart-html/AnchorElement-class.html


Thanks for doing this! It really makes the markdown read much better.

chalin · 2017-11-21T13:41:56Z

@filiph, thanks for the quick feedback! Changes submitted.

Contributes to dart-lang#407 and dart-lang#416. Continuation of dart-lang#440

Contributes to #407 and #416. Continuation of #440

Library tour example tests, up to but excluding dart:html

bce8c72

Contributes to dart-lang#407 and dart-lang#416.

chalin added Docs fix.examples Adds or changes example test.general Relates to unit, integration, perf testing labels Nov 20, 2017

chalin requested a review from filiph November 20, 2017 20:09

googlebot added the cla: yes Contributor has signed the Contributor License Agreement label Nov 20, 2017

chalin commented Nov 20, 2017

View reviewed changes

filiph approved these changes Nov 21, 2017

View reviewed changes

Post-review edits

c9f20b2

chalin merged commit 783590c into dart-lang:master Nov 21, 2017

chalin deleted the chalin-lib-tour-1119 branch November 21, 2017 13:45

chalin added a commit to chalin/site-www that referenced this pull request Nov 21, 2017

Library tour example tests from dart:html onwards

c1451ce

Contributes to dart-lang#407 and dart-lang#416. Continuation of dart-lang#440

chalin added a commit to chalin/site-www that referenced this pull request Nov 22, 2017

Library tour example tests from dart:html onwards

8fc1d7a

Contributes to dart-lang#407 and dart-lang#416. Continuation of dart-lang#440

chalin added a commit to chalin/site-www that referenced this pull request Nov 22, 2017

Library tour example tests from dart:html onwards

ad8e76b

Contributes to dart-lang#407 and dart-lang#416. Continuation of dart-lang#440

chalin mentioned this pull request Nov 22, 2017

Library tour example tests for dart:html #443

Merged

chalin added a commit that referenced this pull request Nov 22, 2017

Library tour example tests from dart:html (#443)

792c517

Contributes to #407 and #416. Continuation of #440

chalin mentioned this pull request Nov 23, 2017

Code excerpts should be auto-updated and tested #407

Closed

20 tasks

chalin mentioned this pull request Dec 12, 2017

Trim down and reorganize examples code #416

Closed

atsansone removed the docs label Oct 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Library tour example tests, up to but excluding dart:html #440

Library tour example tests, up to but excluding dart:html #440

chalin commented Nov 20, 2017

chalin commented Nov 20, 2017

chalin Nov 20, 2017

filiph Nov 21, 2017

chalin Nov 21, 2017

chalin Nov 20, 2017

filiph Nov 21, 2017

chalin Nov 21, 2017

filiph left a comment

filiph Nov 20, 2017

chalin Nov 21, 2017

filiph Nov 21, 2017

chalin Nov 21, 2017

filiph Nov 21, 2017

filiph Nov 21, 2017

chalin Nov 21, 2017

filiph Nov 21, 2017

chalin Nov 21, 2017

kwalrath Dec 9, 2017

chalin Dec 14, 2017

filiph Nov 21, 2017

filiph Nov 21, 2017

filiph Nov 21, 2017

chalin Nov 21, 2017

filiph Nov 21, 2017

chalin Nov 21, 2017

kwalrath Dec 9, 2017

filiph Nov 21, 2017

chalin commented Nov 21, 2017

Library tour example tests, up to but excluding dart:html #440

Library tour example tests, up to but excluding dart:html #440

Conversation

chalin commented Nov 20, 2017

chalin commented Nov 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filiph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalin commented Nov 21, 2017