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.

Sign up for GitHub

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

Jump to bottom

Strong mode code excerpt tests for main page #590

Merged
merged 3 commits into from
Feb 21, 2018
Merged

Strong mode code excerpt tests for main page #590

merged 3 commits into from
Feb 21, 2018

Conversation

chalin
Copy link
Contributor

@chalin chalin commented Feb 20, 2018

@chalin chalin added infra.structure Relates to the tools that create dart.dev SoundDart test.general Relates to unit, integration, perf testing labels Feb 20, 2018
@chalin chalin requested a review from kwalrath February 20, 2018 19:29
@googlebot googlebot added the cla: yes Contributor has signed the Contributor License Agreement label Feb 20, 2018
@chalin chalin force-pushed the chalin-strong-mode-tests-0216 branch from 3f330c5 to 2201a36 Compare February 20, 2018 19:31
@chalin chalin force-pushed the chalin-strong-mode-tests-0216 branch from 2201a36 to f7f64cc Compare February 20, 2018 19:38
kwalrath
kwalrath approved these changes Feb 21, 2018
View reviewed changes
Copy link
Contributor

@kwalrath kwalrath left a comment

Choose a reason for hiding this comment

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

Thank you for taking such care with this page and finding those issues! I have a few suggestions for wording, but otherwise LGTM.

examples/misc/lib/language_tour/built_in_types.dart
@@ -6,6 +6,7 @@ void miscDeclAnalyzedButNotTested() {
// #docregion integer-literals
int x = 1;
int hex = 0xDEADBEEF;
// ignore_for_file: 2, integer_literal_out_of_range
int bigInt = 34653465834652437659238476592374958739845729;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove/alter bigInt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. How about we keep it for this PR, and then generally decide how to document https://github.com/dart-lang/sdk/blob/master/sdk/lib/core/bigint.dart?

src/_guides/language/sound-dart.md Outdated
checkbox.
Gist: https://gist.github.com/3c7c95683f0c06be8326a2fd3975cd19
DartPad url: https://dartpad.dartlang.org/3c7c95683f0c06be8326a2fd3975cd19
Note: Can't use embedded DP because DP
Copy link
Contributor

Choose a reason for hiding this comment

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

DP -> DartPad (even though it's not visible...)

Also... Let's use a string (waiting-for-dartpad-2?) to make it easy to find DartPad-worthy places that are waiting for full Dart 2 support.

src/_guides/language/sound-dart.md Outdated

class Cat extends Animal {}
For example, the following code throws an exception at runtime because it is an error
Copy link
Contributor

Choose a reason for hiding this comment

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

"For example, ..." seems out of context with the warning moved up there.

Let's move the aside to be after the code.

src/_guides/language/sound-dart.md Outdated
{% endprettify %}

In the last example, `x` is inferred as double using downward information.
The return type of the closure is inferred as int using upwards information.
Copy link
Contributor

Choose a reason for hiding this comment

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

GLOBAL: upwards -> upward

(Not your spelling, I know, but the doc uses downward elsewhere, and we should be consistent. Since https://developers.google.com/style/word-list specifies backward instead of backwards, I've taken to favoring "ward" without the "s" for similar words.)

src/_guides/language/sound-dart.md Outdated
{% endprettify %}

In the last example, `x` is inferred as double using downward information.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should either use code font for double and int here, or not code font <int>.

src/_guides/language/sound-dart.md Outdated
{% endprettify %}

In the last example, `x` is inferred as double using downward information.
The return type of the closure is inferred as int using upwards information.
The type argument to `map()` is inferred as `<int>` using upwards information.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we connect these a bit more? Here's a try:

The return type.... Dart uses this return type as upward information when inferring the map() method's type argument: <int>.

src/_guides/language/sound-dart.md

feedAnimals(myCats);
}
List<Cat> myCats = new List<[!MaineCoon!]>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Before this, please add the comment that was there before:

  // Was: List<Cat> myCats = new List<Cat>();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure but I don't think that it is relevant to the presentation. What is being contrasted in the code excerpt is the type argument to List on the LHS vs. the type argument to List on the RHS.

I'll leave out the comment for now and I'll revisit later if you feel the need.

Copy link
Contributor

@kwalrath kwalrath Feb 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The text "In the following example, you can substitute new List<Cat>() with new List<MaineCoon>()" refers to code (new List<Cat>()) that I actually looked above to find, since it seemed to be saying that I'd seen that code before. Having the comment there made me not bother searching. We could also just word it differently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you are right. #607 fixes that. (Given that #607 is a minor fix to this approved PR, I'll assume that it is ok to merge it once Travis is green.)

@chalin chalin merged commit 7af5d7d into master Feb 21, 2018
@chalin chalin deleted the chalin-strong-mode-tests-0216 branch February 21, 2018 14:50
@chalin chalin mentioned this pull request Feb 23, 2018
Closed
20 tasks
chalin added a commit that referenced this pull request Feb 23, 2018
@chalin
Strong mode page: show actual analyzer error
e4ed1e6 
Followup to #590, #603.
@chalin chalin mentioned this pull request Feb 23, 2018
Merged
chalin added a commit that referenced this pull request Feb 23, 2018
@chalin
Strong mode page: show actual analyzer error (#605)
371e099 
Followup to #590, #603.
chalin added a commit that referenced this pull request Feb 23, 2018
@chalin
Strong mode prose tweak
7bb3fea 
Followup to #590 (comment)
@chalin chalin mentioned this pull request Feb 23, 2018
Merged
chalin added a commit that referenced this pull request Feb 26, 2018
@chalin
Strong mode prose tweak (#607)
51b679f 
Followup to #590 (comment)
@atsansone atsansone added dev.null-safety Relates to transforming or migrating Dart code to sound null safety and removed SoundDart labels Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kwalrath kwalrath kwalrath approved these changes

Assignees
No one assigned
Labels
cla: yes Contributor has signed the Contributor License Agreement dev.null-safety Relates to transforming or migrating Dart code to sound null safety infra.structure Relates to the tools that create dart.dev test.general Relates to unit, integration, perf testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@chalin @kwalrath @atsansone @googlebot