<Widget> boilerplate for Flutter `children:` suggestions.

<Widget> boilerplate for Flutter `children:` suggestions.

Cleanup (tracked in TODOs) and consolidation to follow.


R=brianwilkerson@google.com, danrubel@google.com

See: https://github.com/flutter/flutter-intellij/issues/832
Committed: https://github.com/dart-lang/sdk/commit/6f846bcd536928d967805b53cf60855301d066cd

[pquitslund, 2017-03-23 18:00:50]

pquitslund@google.com changed reviewers:
+ brianwilkerson@google.com, danrubel@google.com

[Brian Wilkerson, 2017-03-23 18:13:17]

https://codereview.chromium.org/2768273003/diff/1/pkg/analysis_server/lib/src...
File
pkg/analysis_server/lib/src/services/completion/dart/arglist_contributor.dart
(right):

https://codereview.chromium.org/2768273003/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/arglist_contributor.dart:296:
ConstructorElement constructorElement = element;
If you don't re-assign 'element' (and it looks like you don't) then you don't
need to cast; you can just use 'element'.

https://codereview.chromium.org/2768273003/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/arglist_contributor.dart:299:
for (ParameterElement param in constructorElement.parameters) {
It's a little confusing to have the loop variable and the parameter have the
same name.

I have to wonder why we're looping through the parameters. Aren't we trying to
get the default value for the parameter named 'param'?

https://codereview.chromium.org/2768273003/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/arglist_contributor.dart:303:
InterfaceType interfaceType = type;
Again, there's no need for the extra local variable or implicit cast.

https://codereview.chromium.org/2768273003/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/arglist_contributor.dart:305:
if (typeArguments.length == 1) {
We probably need to guard against a type argument of type 'dynamic'. Try a test
case where the parameter has type 'List' (with no type arguments).

[danrubel, 2017-03-23 18:41:31]

https://codereview.chromium.org/2768273003/diff/1/pkg/analysis_server/test/se...
File
pkg/analysis_server/test/services/completion/dart/arglist_contributor_test.dart
(right):

https://codereview.chromium.org/2768273003/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/dart/arglist_contributor_test.dart:746:
defaultArgListString: '<Widget>[]');
It would be good to have some negative tests... 
1) if the "children" parameter is a Map then defaultArgListString should be null
or empty
2) if the List is of type dynamic as Brian pointed out
3) if completing a method with a 'children:' parameter

[pquitslund, 2017-03-23 20:23:08]

On 2017/03/23 18:41:31, danrubel wrote:
>
https://codereview.chromium.org/2768273003/diff/1/pkg/analysis_server/test/se...
> File
>
pkg/analysis_server/test/services/completion/dart/arglist_contributor_test.dart
> (right):
> 
>
https://codereview.chromium.org/2768273003/diff/1/pkg/analysis_server/test/se...
>
pkg/analysis_server/test/services/completion/dart/arglist_contributor_test.dart:746:
> defaultArgListString: '<Widget>[]');
> It would be good to have some negative tests... 
> 1) if the "children" parameter is a Map then defaultArgListString should be
null
> or empty
> 2) if the List is of type dynamic as Brian pointed out
> 3) if completing a method with a 'children:' parameter

Tests added.

Thanks!

[pquitslund, 2017-03-23 20:23:40]

Updated. PTAL!

https://codereview.chromium.org/2768273003/diff/1/pkg/analysis_server/lib/src...
File
pkg/analysis_server/lib/src/services/completion/dart/arglist_contributor.dart
(right):

https://codereview.chromium.org/2768273003/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/arglist_contributor.dart:296:
ConstructorElement constructorElement = element;
On 2017/03/23 18:13:17, Brian Wilkerson wrote:
> If you don't re-assign 'element' (and it looks like you don't) then you don't
> need to cast; you can just use 'element'.

Done.

https://codereview.chromium.org/2768273003/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/arglist_contributor.dart:299:
for (ParameterElement param in constructorElement.parameters) {
On 2017/03/23 18:13:17, Brian Wilkerson wrote:
> It's a little confusing to have the loop variable and the parameter have the
> same name.
> 
> I have to wonder why we're looping through the parameters. Aren't we trying to
> get the default value for the parameter named 'param'?

Quite right!  I was trying to repurpose this with an eye towards consolidating
(and reusing) down the road but yeah, this is silly.  Fixed.

Thanks!

https://codereview.chromium.org/2768273003/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/arglist_contributor.dart:303:
InterfaceType interfaceType = type;
On 2017/03/23 18:13:17, Brian Wilkerson wrote:
> Again, there's no need for the extra local variable or implicit cast.

Done.

https://codereview.chromium.org/2768273003/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/arglist_contributor.dart:305:
if (typeArguments.length == 1) {
On 2017/03/23 18:13:17, Brian Wilkerson wrote:
> We probably need to guard against a type argument of type 'dynamic'. Try a
test
> case where the parameter has type 'List' (with no type arguments).

Done.

[Brian Wilkerson, 2017-03-23 20:27:53]

lgtm

https://codereview.chromium.org/2768273003/diff/40001/pkg/analysis_server/lib...
File
pkg/analysis_server/lib/src/services/completion/dart/arglist_contributor.dart
(right):

https://codereview.chromium.org/2768273003/diff/40001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/completion/dart/arglist_contributor.dart:275:
//TODO(pq): unify with `utilities.dart`
Do these TODOs still apply?

[danrubel, 2017-03-23 20:28:44]

LGTM

[pquitslund, 2017-03-23 22:15:16]

Thanks!

https://codereview.chromium.org/2768273003/diff/40001/pkg/analysis_server/lib...
File
pkg/analysis_server/lib/src/services/completion/dart/arglist_contributor.dart
(right):

https://codereview.chromium.org/2768273003/diff/40001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/completion/dart/arglist_contributor.dart:275:
//TODO(pq): unify with `utilities.dart`
On 2017/03/23 20:27:53, Brian Wilkerson wrote:
> Do these TODOs still apply?

They may.  There's a bit of duplication I want to take a pass it in any event
and I'll sweep these up then.

Thanks!

[pquitslund, 2017-03-23 22:20:49]

Description was changed from

==========
<Widget> boilerplate for Flutter `children:` suggestions.

Cleanup (tracked in TODOs) and consolidation to follow.


See: https://github.com/flutter/flutter-intellij/issues/832
==========

to

==========
<Widget> boilerplate for Flutter `children:` suggestions.

Cleanup (tracked in TODOs) and consolidation to follow.


R=brianwilkerson@google.com, danrubel@google.com

See: https://github.com/flutter/flutter-intellij/issues/832
Committed:
https://github.com/dart-lang/sdk/commit/6f846bcd536928d967805b53cf60855301d066cd
==========

[pquitslund, 2017-03-23 22:20:50]

Committed patchset #3 (id:40001) manually as
6f846bcd536928d967805b53cf60855301d066cd (presubmit successful).