-
Notifications
You must be signed in to change notification settings - Fork 724
Dart 2 changes to the language tour #677
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great changes! Please see inlined comments.
@@ -6,8 +6,6 @@ void miscDeclAnalyzedButNotTested() { | |||
// #docregion integer-literals | |||
int x = 1; | |||
int hex = 0xDEADBEEF; | |||
// ignore_for_file: 2, integer_literal_out_of_range | |||
int bigInt = 34653465834652437659238476592374958739845729; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[No action necessary] We should document BigInt
somewhere under "Libraries". I've added a bullet to our meta issue #637.
such as type safety, optional `new` and `const`, | ||
and changes to assertion support. | ||
</div> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an alert a few lines below that mentions using Dartpad. We should probably comment that out until Dartpad catches up.
Alternatively, use /\* ... \*/. For details, see | ||
[Comments](#comments). | ||
: A way to make Dart ignore some content. | ||
For more information, see [Comments](#comments). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about making this entry consistent with the ones that follow, something like:
A single-line comment. Dart also supports multi-line and document comments. See Comments for details.
: A type. Some of the other built-in types are String, int, and bool. | ||
: A type. A `num` variable's value can be either an `int` or a | ||
`double` object. | ||
Some of the other built-in types are `String`, `List`, and `bool`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest that we use int
to illustrate a type (in the prose here and the code excerpt at the start of this section. That way we can simplify this entry to read:
int
: A type. Some of the other built-in types are
String
,List
, andbool
.
Also, it would align more closely to the prose, a little further below, of number
being inferred as an int
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I struggled with whether to change this. I wasn't sure why we used num
in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that maybe we saw JavaScript programmers using no type when they should've used num
. With type safety, that shouldn't be as much of an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @sethladd in case his memory is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm renaming the function printInteger
and changing the type to int
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works for me! :)
in development, rather than at runtime. Strong mode is optional in | ||
Dart 1.x, but not optional in [Dart 2](/dart-2). | ||
- Although Dart is strongly typed, type annotations are optional | ||
because Dart can often infer the type. In the code above, `number` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--> because Dart can infer types.
|
||
{% include optional-types-2.0.html %} | ||
Generics are often required for type safety, but they have other benefits | ||
than just allowing your code to run: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other benefits --> more benefits
(otherwise the rest of the sentence doesn't work)
the list is probably a mistake. Here’s an example: | ||
|
||
<?code-excerpt "misc/test/language_tour/generics_test.dart (why-generics)"?> | ||
{% prettify dart %} | ||
var names = new List<String>(); | ||
names.addAll(['Seth', 'Kathy', 'Lars']); | ||
names.add(42); // Fails in checked mode (succeeds in production mode). | ||
names.add(42); // Error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Drop the period:
- // Error. --> // Error
(I believe that this change was only done in this file, not the original source.)
{% comment %} | ||
TODO: Confirm that non-SomeBaseClass is an error. | ||
Perhaps be more specific. | ||
{% endcomment %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confirm that it is an error: 'Object' doesn't extend 'SomeBaseClass'
. I'd suggest pulling var objectFoo = new Foo<Object>();
out into it's own excerpt, uncommented, so that we can have the analyzer validate our claim that it is an error.
Would you like for me to make that change in this, or maybe a followup, PR?
If you'd like we can have the analyzer error automatically inlined in the prose (as we've done elsewhere in the docs), but I don't think that is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if you could write that test, I'd appreciate it. My test fu isn't great. (I agree we don't need to inline the error.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working on the test now.
it doesn't wait for those operations. | ||
Instead, the async function executes only until it encounters | ||
its first `await` expression. | ||
Then it returns a Future object, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Style nit] Future --> Future
(I'm not sure that we are consistent with this convention. E.g., one of the changes you made early in this page was to use <code> style for String
, int
, etc. but I don't think that we are systematic about it.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't consistent. At first I was trying to minimize the use of code font, especially in words that are already capitalized (this was the style at NeXT, but code font keeps creeping in.
Although an async function might perform time-consuming operations, | ||
it doesn't wait for those operations. | ||
Instead, the async function executes only until it encounters | ||
its first `await` expression. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good note. We should be able to refer the reader to a page with more details. How about using the link to https://github.com/dart-lang/sdk/blob/master/docs/newsletter/20170915.md#synchronous-async-start for now?
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
FYI, I've committed a fix for analysis failure (adjusting expected analyzer output). |
Thanks for the review and the commit! I'll update this soon. |
Thank you for updating the tour to Dart 2!! Let us know how we can help advertise it when it's published :) |
Also alphabetized the list o' links.
ptal |
During development, you can validate inputs by using `assert` in the | ||
initializer list. | ||
|
||
<?code-excerpt "misc/lib/language_tour/classes/extends.dart" replace="/extends|super/[!$&!]/g"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a superfluous <?code-excerpt?>
instruction. I've removed it.
Re. #677 (comment): I've added the test; see the last commit. I've had to adjust the prose of that section. It now read as follows: |
LGTM. Thanks, Patrice! |
Contributes to #507. Staged at:
https://kw-www-dartlang-1.firebaseapp.com/guides/language/language-tour
Even though this page isn't completely done, if it's acceptable I'd like to commit and iterate.