Setup language-tour.md for auto code-refresh from examples/language-tour sources #414

chalin · 2017-11-01T21:40:15Z

Reviewers:

You'll probably want to focus your review efforts on src/_guides/language/language-tour.md while ignoring whitespace diffs:

The code diffs that you will see are the result of having refreshed the code excerpts from their originating sources. (After this PR is merged, I'll set this up so that it happens automatically.)
In very few places I adjusted the Tour prose and/or examples -- but usually only after discussing the proposed change with you via email.

TL;DR

The purpose of this PR has been to adapt the Language Tour, and its example sources, so that we can use the code_excerpt_updater to auto-refresh code excerpts in the Language Tour markdown.

For the most part, the language tour markdown already contained embedded paths to the source files it uses, and the source files contained some form of code excerpt markers. Consequently, most changes made in this PR are "lexical" and minor:

Sources under examples/language-tour: changed code excerpt marker syntax from
- BEGIN/END to
- #docregion/#enddocregion.
src/_guides/language/language-tour.md: changed inline code excerpt comments
-  to
- <?code-excerpt? "path/to/source"?> processing instructions.

Doc regions were manually adjusted to ensure that the code excerpts remained unchanged (except for genuine updates done to the code since the markdown was written).

In a few cases files were relocated because they were in a wrong folder (e.g., deprecated was moved to metadata).

Contributes to #407

What is left to do: I haven't touched code excepts that are in the Gists used with DartPad.

kevmoo

In general it looks good, except for all of the types being added.

Especially within a function, var is great!

What's the reasoning here?

kevmoo · 2017-11-01T21:50:52Z

src/_guides/language/language-tour.md

 {% prettify dart %}
-final name = 'Bob'; // Or: final String name = 'Bob';
+final String name = 'Bob'; // Or: final String name = 'Bob';


Why adding types?

Type annotations like these are appearing because the code excerpt is now synced with its originating source file. dart-archive/dart-up-and-running-book#53 is the PR in which most of these type annotations were introduced.

Here is a direct link to the change made to the code excerpt you commented on: https://github.com/dart-lang/dart-up-and-running-book/pull/53/files#diff-9c88f184c7d08c6ebcfb3ff1909212db:

- final name = 'Bob'; // Or: final String name = 'Bob'; + final String name = 'Bob'; // Or: final String name = 'Bob'; // name = 'Alice'; // Uncommenting this causes an error // END(final_initialization) - final name2 = 'Roberta'; + final String name2 = 'Roberta'; ...

In this, and many other cases, I believe that type annotations were added because the type_annotate_public_apis analyzer rule.

If this isn't what we want, then we'll need to drop that analyzer rule and fix the source files. Let me know what you prefer.

This is kinda tough.

If we're just showing folks "here's how you set a variable", that code – IMHO – should be in a main method or something.

These are more tricky. v2 inference will mean even public fields – like these – won't strictly need types.

@munificent – thoughts here?

... that code – IMHO – should be in a main method or something.

Agreed. I can review each of the code diffs (that add extra type annotations), and remove the annotation if it isn't meaningful for that particular example.

kevmoo · 2017-11-01T21:50:59Z

src/_guides/language/language-tour.md

 {% prettify dart %}
-const bar = 1000000;       // Unit of pressure (dynes/cm2)
-const atm = 1.01325 * bar; // Standard atmosphere
+const int bar = 1000000; // Unit of pressure (dynes/cm2)


....and here?

kevmoo · 2017-11-01T21:51:17Z

src/_guides/language/language-tour.md

-var s2 = "Double quotes work just as well.";
-var s3 = 'It\'s easy to escape the string delimiter.';
-var s4 = "It's even easier to use the other delimiter.";
+String s1 = 'Single quotes work well for string literals.';


...and here...

These variable declarations appear in the section on "Strings", which may be why the variables are declared as String. (The same is true of the use of int and double above, in the sections on "Numbers".)

But — the original page doesn't have the type annotations, right? https://www.dartlang.org/guides/language/language-tour#strings

Let's keep it that way.

Let's keep it that way.

Ok, I'll revert those code changes that were done before the code was copied over into this repo.

chalin · 2017-11-02T01:49:34Z

src/_guides/language/language-tour.md

 {% prettify dart %}
-var list = [1, 2, 3];
+List<int> list = [1, 2, 3];


Ditto: IMHO, this type annotation helps emphasize that we are in the section talking about Lists.

I would just leave var.

chalin · 2017-11-02T02:11:49Z

PTAL at the latest commit. I've removed only two type annotations: for each of the examples of final and const, I've left one declaration w/o a type and kept one declaration with a type annotation.

I've looked at all other cases, and the places where type annotations were added make sense to me (see my inline comments). Let me know what you think.

kwalrath · 2017-11-06T22:31:56Z

I don't think we should use types anywhere that we wouldn't in our own well-written code. If need be, we can mention the type in a comment or the text.

filiph

LGTM with comments. I don't feel strongly about it but I would suggest removing the added type annotations.

filiph · 2017-11-07T01:06:00Z

examples/analysis_options.yaml

-    - language-tour/reference/*.dart
-    - library-tour/reference/*.dart
-    - library-tour/async-await/bin/main.dart
+  # exclude:


Why not just remove these?

filiph · 2017-11-07T01:10:16Z

src/_guides/language/language-tour.md

 {% prettify dart %}
 // Define a function.
-printNumber(num aNumber) {
+void printNumber(num aNumber) {


I think, in language tour, we can actually make omissions like this (no void) for the sake of smoother beginnings. Does that break the code-excerpt pipeline? If this is because of a lint, maybe we want to use // ignore_for_file annotations?

I think, in language tour, we can actually make omissions like this (no void) for the sake of smoother beginnings.

Ok

Does that break the code-excerpt pipeline?

Not at all.

If this is because of a lint, ...

Yes. I'm thinking that we have too many lint rules. Maybe we should opt for the same list that we recommend for Angular: https://github.com/dart-lang/stagehand/blob/master/templates/web-angular/analysis_options.yaml

@kwalrath @filiph WDYT?

I think in general it's good to have as many lint rules turned on for samples as possible (because by default, samples should be exceptionally "standard" code). I'd rather we keep them on but that we're not afraid to use // ignore_for_file where it makes sense.

filiph · 2017-11-07T01:12:00Z

src/_guides/language/language-tour.md

-var s2 = "Double quotes work just as well.";
-var s3 = 'It\'s easy to escape the string delimiter.';
-var s4 = "It's even easier to use the other delimiter.";
+String s1 = 'Single quotes work well for string literals.';


But — the original page doesn't have the type annotations, right? https://www.dartlang.org/guides/language/language-tour#strings

Let's keep it that way.

filiph · 2017-11-07T01:13:27Z

src/_guides/language/language-tour.md

 {% prettify dart %}
-var list = [1, 2, 3];
+List<int> list = [1, 2, 3];


I would just leave var.

Code excerpts are preceded by an HTML comment containing a path to the file from which the excerpt originates. Replace each such path with a `<?code-excerpt?>` processing instruction. This results from a global search and replace. Corrections will follow.

Adjust / set `#docregion`s in Language Tour examples sources, so that we can properly extract the code excerpts into the language-tour.md.

Adjusted all `<?code-excerpts?>` instructions to ensure that they are referring to the right source file and region. Then did a refresh of all code excerpts.

chalin · 2017-11-08T01:27:21Z

src/_guides/language/language-tour.md

+  [Error]({{site.dart_api}}/dart-core/Error-class.html) or
+  [Exception]({{site.dart_api}}/dart-core/Exception-class.html).
+</div>
+


@kwalrath - I added and "info alert" based on @munificent's suggestion. WDYT?

Looks good, but let's rewrite slightly to be more precise and avoid the unnecessary future tense:

Note: Production-quality code usually throws types that...

kwalrath

I'd like one small change, but otherwise LGTM. Thanks for doing this!

kwalrath · 2017-11-08T05:26:35Z

src/_guides/language/language-tour.md

+  [Error]({{site.dart_api}}/dart-core/Error-class.html) or
+  [Exception]({{site.dart_api}}/dart-core/Exception-class.html).
+</div>
+


Looks good, but let's rewrite slightly to be more precise and avoid the unnecessary future tense:

Note: Production-quality code usually throws types that...

chalin added infra.structure Relates to the tools that create dart.dev a.libraries Relating to the Dart standard libraries. labels Nov 1, 2017

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

chalin requested review from kevmoo and filiph November 1, 2017 21:44

kevmoo reviewed Nov 1, 2017

View reviewed changes

chalin commented Nov 2, 2017

View reviewed changes

filiph approved these changes Nov 7, 2017

View reviewed changes

chalin added 8 commits November 7, 2017 19:11

Fix invalid package names

5195244

language-tour.md: fix code-example path errors, add missing source file

d5ac89d

example/language-tour: convert BEGIN/END to #docregion syntax

0b07ad8

examples/language-tour: adjust code #docregions

81ec1d3

Adjust / set `#docregion`s in Language Tour examples sources, so that we can properly extract the code excerpts into the language-tour.md.

Jekyll prettify plugin: support [!...!] highlight syntax

8a31784

language-tour.md: auto-refresh of all code excerpts

29b2acd

Adjusted all `<?code-excerpts?>` instructions to ensure that they are referring to the right source file and region. Then did a refresh of all code excerpts.

Post-review edits, and renamed analysis options file

7c25833

chalin force-pushed the chalin-language-tour-code-sync-1101 branch from 0fa5032 to ee3d624 Compare November 8, 2017 01:23

More post-review edits mainly based on Filip's feedback

edc38df

chalin force-pushed the chalin-language-tour-code-sync-1101 branch from ee3d624 to edc38df Compare November 8, 2017 01:25

chalin commented Nov 8, 2017

View reviewed changes

kwalrath approved these changes Nov 8, 2017

View reviewed changes

Final tweak to a note

bfc7a2b

chalin merged commit 05cc6f3 into dart-lang:master Nov 8, 2017

chalin deleted the chalin-language-tour-code-sync-1101 branch November 8, 2017 13:10

chalin mentioned this pull request Nov 23, 2017

Code excerpts should be auto-updated and tested #407

Closed

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setup language-tour.md for auto code-refresh from examples/language-tour sources #414

Setup language-tour.md for auto code-refresh from examples/language-tour sources #414

chalin commented Nov 1, 2017 •

edited

Loading

kevmoo left a comment

kevmoo Nov 1, 2017

chalin Nov 1, 2017

kevmoo Nov 2, 2017

chalin Nov 2, 2017

kevmoo Nov 1, 2017

kevmoo Nov 1, 2017

chalin Nov 2, 2017

filiph Nov 7, 2017

chalin Nov 8, 2017 •

edited

Loading

chalin Nov 2, 2017

filiph Nov 7, 2017

chalin Nov 8, 2017

chalin commented Nov 2, 2017

kwalrath commented Nov 6, 2017

filiph left a comment

filiph Nov 7, 2017

chalin Nov 8, 2017

filiph Nov 7, 2017

chalin Nov 8, 2017

filiph Nov 8, 2017

chalin Nov 8, 2017

filiph Nov 7, 2017

filiph Nov 7, 2017

chalin Nov 8, 2017

kwalrath Nov 8, 2017

chalin Nov 8, 2017

kwalrath left a comment

kwalrath Nov 8, 2017

Setup language-tour.md for auto code-refresh from examples/language-tour sources #414

Setup language-tour.md for auto code-refresh from examples/language-tour sources #414

Conversation

chalin commented Nov 1, 2017 • edited Loading

kevmoo 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

chalin Nov 8, 2017 • edited Loading

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 2, 2017

kwalrath commented Nov 6, 2017

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

kwalrath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalin commented Nov 1, 2017 •

edited

Loading

chalin Nov 8, 2017 •

edited

Loading