Skip to content
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

Embrace optional new/const in the language tour #940

Merged
merged 17 commits into from
Jun 15, 2018
Merged

Conversation

kwalrath
Copy link
Contributor

@kwalrath kwalrath requested a review from chalin June 15, 2018 19:39
@googlebot googlebot added the cla: yes Contributor has signed the Contributor License Agreement label Jun 15, 2018
@chalin
Copy link
Contributor

chalin commented Jun 15, 2018

Hmm, if you open the staged page's Console, there are lots of errors there.

@chalin
Copy link
Contributor

chalin commented Jun 15, 2018

Rebasing (to get #941), will help with the Travis errors.

// #enddocregion const-context-noconst

// Same instances!
assert(identical(pointAndLine1, pointAndLine2));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestions:

  • Use expect()
  • const values are canonicalized, so you can use ==

E.g.,

expect(pointAndLine1 == pointAndLine2, isTrue);
...

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looking good so far. Can we get Travis green before I do a detailed review?

@chalin
Copy link
Contributor

chalin commented Jun 15, 2018

This is what I see in the JS console (maybe these are from the embedded DartPad? cc @jcollins-g):
screen shot 2018-06-15 at 16 36 20

@kwalrath
Copy link
Contributor Author

I see what looks like the same console errors in https://www.dartlang.org/guides/language/language-tour. I think we need to remove all the embedded DartPads (in another PR) to fix this (#422).

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Only a few (inlined) comments/questions.

add back the comments that are still relevant.
4. Run `./scripts/analyze-and-test-examples.sh` to confirm that
your changes are good.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding these helpful instructions.

{% prettify dart %}
final p = const ImmutablePoint(2, 2);
{% endprettify %}

Constructing two identical compile-time constants results in a single,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that this is from before, but maybe we should link to identical() here?

The definition is already given at the end of this file:

[identical()]: {{site.dart_api}}/{{site.data.pkg-vers.SDK.channel}}/dart-core/identical.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to leave this out, because I'd rather not add more words.


<aside class="alert alert-info" markdown="1">
**Version note:** The `const` keyword became optional
(within a constant context) in Dart 2.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe drop the parentheses?

@@ -61,7 +66,7 @@ void main() {
test('const, identical, runtimeType', () {
_test() {
// #docregion const
var p = const ImmutablePoint(2, 2);
final p = const ImmutablePoint(2, 2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change from var to final?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if you'd catch that. :) I don't remember why I changed it. I think it was a side effect of the DartPad changes I was making. I'll change it back.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)
👍

@kwalrath kwalrath merged commit 23aac2f into master Jun 15, 2018
@kwalrath kwalrath deleted the kw-lt-denewing branch June 15, 2018 22:21
@jcollins-g
Copy link
Contributor

@kwalrath Removing all the embedded dartpads seems like an unfortunate consequence.

The errors look like they might be a consequence of having an embedded Dartpad, but exactly why isn't clear. Filing an issue.

@jcollins-g
Copy link
Contributor

Added more information about the new errors to dart-lang/dart-pad#791.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Contributor has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants