Arglist `children:` default value gen fixes.

Arglist `children:` default value gen fixes.

* fixed arglist codegen (include params)
* fixed defaultTextRange generation (and added tests)
* cleanup (followup from https://codereview.chromium.org/2768273003/


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

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

[pquitslund, 2017-03-23 23:10:56]

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

[Brian Wilkerson, 2017-03-23 23:34:48]

lgtm

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

https://codereview.chromium.org/2771123002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/utilities.dart:86:
sb.write(defaultValue);
'defaultValue' can be null here.

https://codereview.chromium.org/2771123002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/utilities.dart:181: String
getDefaultStringParameterValue(ParameterElement param) {
Consider passing in a StringSink here rather than returning a value that has to
be null-checked (and changing the name of the method as appropriate).

[danrubel, 2017-03-24 13:16:19]

LGTM

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

https://codereview.chromium.org/2771123002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/utilities.dart:76:
ClassElement classElement = element.enclosingElement;
It appears that "element" is not modified in this method, thus you can use
"element" as a ClassElement within this "if" statement and remove
"classElement".

[pquitslund, 2017-03-24 15:56:23]

Thanks!

I've got another change on the way that will improve testing.

More soon.

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

https://codereview.chromium.org/2771123002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/utilities.dart:76:
ClassElement classElement = element.enclosingElement;
On 2017/03/24 13:16:19, danrubel wrote:
> It appears that "element" is not modified in this method, thus you can use
> "element" as a ClassElement within this "if" statement and remove
> "classElement".

Done.

https://codereview.chromium.org/2771123002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/utilities.dart:86:
sb.write(defaultValue);
On 2017/03/23 23:34:48, Brian Wilkerson wrote:
> 'defaultValue' can be null here.

Right.  I was thinking this was OK since the `toString()` of null is "null"
(which is I *think* what we want).

I'll add a test though.

https://codereview.chromium.org/2771123002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/utilities.dart:181: String
getDefaultStringParameterValue(ParameterElement param) {
On 2017/03/23 23:34:47, Brian Wilkerson wrote:
> Consider passing in a StringSink here rather than returning a value that has
to
> be null-checked (and changing the name of the method as appropriate).

Interesting idea.  As mentioned above, I don't think the null check is necessary
unless I'm confused.  In any event some tests are a good idea and I have a
parallel change that addresses test fixes in general and will fill this out a
bit more.

To be continued... :)

[Brian Wilkerson, 2017-03-24 16:04:32]

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

https://codereview.chromium.org/2771123002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/utilities.dart:86:
sb.write(defaultValue);
Given a widget class with a constructor of the form:

  Widget2({Map<String, Widget> children});

If the user requests code completion here:

  Widget2(^)

the suggestion will be "children: null". If we can't do better than this, do we
really want this, or should we call back to the behavior used everywhere else of
just suggesting "children: "? (I'd vote for the latter, but you might want a
second opinion :-)

[pquitslund, 2017-03-24 16:36:30]

Description was changed from

==========
Arglist `children:` default value gen fixes.

* fixed arglist codegen (include params)
* fixed defaultTextRange generation (and added tests)
* cleanup (followup from https://codereview.chromium.org/2768273003/


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

to

==========
Arglist `children:` default value gen fixes.

* fixed arglist codegen (include params)
* fixed defaultTextRange generation (and added tests)
* cleanup (followup from https://codereview.chromium.org/2768273003/


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

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

[pquitslund, 2017-03-24 16:36:31]

Committed patchset #2 (id:20001) manually as
4a9cabf493600a6b0a002c8a5edf1a9b91253919 (presubmit successful).

[pquitslund, 2017-03-24 16:37:45]

On 2017/03/24 16:04:32, Brian Wilkerson wrote:
>
https://codereview.chromium.org/2771123002/diff/1/pkg/analysis_server/lib/src...
> File pkg/analysis_server/lib/src/services/completion/dart/utilities.dart
> (right):
> 
>
https://codereview.chromium.org/2771123002/diff/1/pkg/analysis_server/lib/src...
> pkg/analysis_server/lib/src/services/completion/dart/utilities.dart:86:
> sb.write(defaultValue);
> Given a widget class with a constructor of the form:
> 
>   Widget2({Map<String, Widget> children});
> 
> If the user requests code completion here:
> 
>   Widget2(^)
> 
> the suggestion will be "children: null". If we can't do better than this, do
we
> really want this, or should we call back to the behavior used everywhere else
of
> just suggesting "children: "? (I'd vote for the latter, but you might want a
> second opinion :-)

I'll pick this up in my other CL that gets this better under test.  I lean
towards the latter too FWIW!