-
Notifications
You must be signed in to change notification settings - Fork 685
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
Strong mode code excerpt tests for fix-it page #603
Conversation
0fb637b
to
8cca216
Compare
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.
A few possible issues and suggestions, but in general this looks great. Thanks for improving the page, as well as making sure the code will be tested!
{% comment %} | ||
NOTE TO Kathy: I don't think that we should recommend use of dynamic in this case, | ||
so I'm commenting this out. WDYT? | ||
|
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.
Is there a similar case where you would want dynamic?
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.
While not for this specific example, there are similar situations where we'd like to be able to write a union type but can't. I'll tweak the code and include the example using dynamic.
NOTE TO Kathy: I think that we should consider renaming this section. How about | ||
<a id ="assigning-mismatched-types"></a> | ||
### Unexpected collection element type | ||
|
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.
SGTM
error • The getter 'context2D' isn't defined for the class 'Element' • undefined_getter | ||
``` | ||
|
||
#### **Fix**: Replace the definition of the member with an explicit type declaration or a downcast |
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.
Let's not boldface "Fix" (global).
class Animal { | ||
void chase(Animal x) {} | ||
void chase(Animal x) { ... } |
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.
It'd be nice to use the NumberAdder example instead of Animal.
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.
Ok, but why do you feel it would be better? (I'll leave the Animal hierarchy for now.)
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 the link is from the section that has the NumberAdder example. It's a bit jarring to switch to another example.
|
||
This sometimes happens when you create a simple dynamic collection | ||
and the analyzer Dart infers the type in a way you didn't expect. | ||
and the analyzer infers the type in a way you didn't expect. | ||
When you later add values of a different type, | ||
the analyzer produces a warning. |
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.
warning -> error
@@ -175,45 +208,56 @@ to specify a type. For more information, see | |||
[Missing type arguments](#missing-type-arguments). | |||
</aside> | |||
|
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.
Above, the note says "warning" but should say "error" or "issue".
For example, the following code initializes a map with several | ||
(String, integer) pairs. The analyzer infers that map to be of type | ||
The following code initializes a map with several | ||
(String, int) pairs. The analyzer infers that map to be of type | ||
`<String, int>` but the code assumes `<String, dynamic>`. |
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 argue that it assumes <String, num>
, which matches the suggested fix. Maybe:
...<String, int>
, but the code seems to assume either <String, dynamic>
or <String, num>
.
However, in Dart 2 passing something other than `dynamic` (or another _top_ | ||
type, such as `Object`, or a specific bottom type, such as `Null`) fails at | ||
compile-time (called `uses_dynamic_as_bottom`): | ||
compile-time. |
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 is just a warning, not an error, so does it really fail?
|
||
One fix could be, if possible, typing `Filter` as `Filter<T>`: | ||
When possible, fix this error by adding type parameters: |
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.
fix this error -> avoid this warning
?
different than the variable's static type. Like most modern statically | ||
typed langauges, Dart accomplishes this with a mixture of static | ||
typed langauges, Dart accomplishes this with a combination of static |
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.
langauges -> languages
Contributes to #407
Staged at https://dartlang-org-staging-0.firebaseapp.com/guides/language/sound-problems
Each of the static warnings/errors now has:
E.g.,